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

Fix/jest resolve module #47180

Merged
merged 4 commits into from
Nov 9, 2020
Merged

Fix/jest resolve module #47180

merged 4 commits into from
Nov 9, 2020

Conversation

scinos
Copy link
Contributor

@scinos scinos commented Nov 6, 2020

Background

#47002 introduced a change where we don't compile packages/ as CJS by default. In those packages, in their package.json usually main points to a CJS compilation and module to a ESM.

This is fine when those packages are used in webpack, as it will read the file from module. However Jest only looks for main, so the tests will fail to resolve those modules.

Change

This PR changes our custom Jest resolver to look for module and then main if the package comes from packages/.

Testing instructions

  • Check all tests are passing
  • Rebase onto add/use-lp-in-calypso, and verify all tests pass when you run yarn test-client.

@matticbot
Copy link
Contributor

@scinos scinos changed the base branch from master to add/use-lp-in-calypso November 6, 2020 09:53
@matticbot
Copy link
Contributor

matticbot commented Nov 6, 2020

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@scinos scinos force-pushed the fix/jest-resolve-module branch from d9574cd to 032108f Compare November 6, 2020 10:11
@scinos scinos requested review from a team as code owners November 6, 2020 10:11
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 6, 2020
@scinos scinos changed the base branch from add/use-lp-in-calypso to master November 6, 2020 10:12
@matticbot
Copy link
Contributor

Caution: This PR affects files in the Editing Toolkit Plugin on WordPress.com
Please ensure your changes work on WordPress.com before merging.

D52363-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing the Editing Toolkit Plugin for more info: PCYsg-ly5-p2

@scinos scinos requested review from a team and removed request for a team November 6, 2020 10:33
@scinos scinos self-assigned this Nov 6, 2020
client/webpack.config.node.js Outdated Show resolved Hide resolved
test/module-resolver.js Outdated Show resolved Hide resolved
Copy link
Member

@jsnajdr jsnajdr left a comment

Choose a reason for hiding this comment

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

This works great for me, even together with #46328. Thanks 👍

There are some suggestions to fix typos and add code comments, but other than that, the PR is ready.

test/module-resolver.js Show resolved Hide resolved
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@scinos scinos merged commit 2e73882 into master Nov 9, 2020
@scinos scinos deleted the fix/jest-resolve-module branch November 9, 2020 09:42
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 9, 2020
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.

4 participants