Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vendure 2.1.1 Migration #271

Merged
merged 26 commits into from
Nov 3, 2023
Merged

Vendure 2.1.1 Migration #271

merged 26 commits into from
Nov 3, 2023

Conversation

dalyathan
Copy link
Member

@dalyathan dalyathan commented Oct 20, 2023

Description

Vendure has been updated to 2.1.1 and dependencies on apollo-server-core has been removed. Because after adding tests for admin-ui compilation the server.destroy() call in afterAll started to timeout, it has been removed.

Breaking changes

Does this PR include any breaking changes we should be aware of? NO

Checklist

📌 Always:

  • I have set a clear title
  • My PR is small and contains only a single feature. (Split into multiple PR's if needed)
  • I have reviewed my own PR, fixed all typo's and removed unused/commented code

⚡ Most of the time:

  • [] I have added or updated test cases for important functionality
  • [] I have updated the README if needed

📦 For publishable packages:

@dalyathan
Copy link
Member Author

The tests seem to be failing because of this

@dalyathan dalyathan changed the title Vendure 2.1.1 Migration WIP: Vendure 2.1.1 Migration Oct 22, 2023
@martijnvdbrug
Copy link
Member

@dalyathan not sure if related, but FYI: https://discord.com/channels/1100672177260478564/1165075107568439326

@dalyathan
Copy link
Member Author

Okay, I will look into it

@dalyathan dalyathan changed the title WIP: Vendure 2.1.1 Migration Vendure 2.1.1 Migration Oct 25, 2023
Copy link
Member

@martijnvdbrug martijnvdbrug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! One thing that comes to mind: have you manually tested admin UI compilation? Should we enable that in e2e tests?

I have done it before, but it took a bit long. However, admin UI compilation is a lot faster, so we could enable compile-tests again with a testcase like this:

  it.skip('Should compile admin', async () => {
    fs.rmSync(path.join(__dirname, '__admin-ui'), {
      recursive: true,
      force: true,
    });
    await compileUiExtensions({
      outputPath: path.join(__dirname, '__admin-ui'),
      extensions: [MyparcelPlugin.ui],
    }).compile?.();
    const files = fs.readdirSync(path.join(__dirname, '__admin-ui/dist'));
    expect(files?.length).toBeGreaterThan(0);
  }, 60000);
});

I guess 60000 (1 minute) should be more than enough, if longer, throw an error?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Useful util, but can you move it to packages/test? packages/util are utilities for usage in plugin runtime code (not-testcode).

@@ -65,9 +64,6 @@ export interface StripeSubscriptionPluginOptions {
...(config.paymentOptions.paymentMethodEligibilityCheckers ?? []),
hasStripeSubscriptionProductsPaymentChecker,
];
config.apiOptions.middleware.push(
createRawBodyMiddleWare('/stripe-subscription*')
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh I missed this one. This should be there, because the request's raw body is needed to validate the incoming signature of Stripe's webhook. The e2e test don't catch this, because for testing I disabled the signature checking with

StripeSubscriptionPlugin.init({
          disableWebhookSignatureChecking: true,
        }),

This was disabled because I couldn't find a way to mock the signature creation properly.

I will look into it.

Copy link
Member

@martijnvdbrug martijnvdbrug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The stripe plugin should have the createRawBodyMiddleware. But I will look into it.

There are some issues where people are experiencing similar issues:

vendure-ecommerce/vendure#2454

vendure-ecommerce/vendure#2473

@martijnvdbrug
Copy link
Member

Actually, let me round up this PR first, because it's a big refactor of the Stripe Subscription plugin.

After that I will review/edit this

@martijnvdbrug
Copy link
Member

@dalyathan I have merged the latest refactored version of Stripe Subscription into this branch, so if you'd like to look at resolving this, then we can merge and publish 👌

Copy link
Member

@martijnvdbrug martijnvdbrug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super nice work 🦾

I re-added rawBodyMiddleware to the Picqer plugin and Sendcloud though, because they need it for the same reason as Stripe

@martijnvdbrug martijnvdbrug merged commit 985a9e6 into main Nov 3, 2023
26 checks passed
@martijnvdbrug martijnvdbrug deleted the feat/vendure-2.1.1-migration branch November 3, 2023 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants