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

Use CJS modules to quell Jest #1130

Merged
merged 2 commits into from
Sep 29, 2020
Merged

Use CJS modules to quell Jest #1130

merged 2 commits into from
Sep 29, 2020

Conversation

cag
Copy link
Contributor

@cag cag commented Sep 29, 2020

So this is one way to allow the tests to pass again, by changing the imports back to CJS. Jest prefers CJS modules when running tests, and by default does not process anything in node_modules with Babel or anything. I'm not sure how it works with the other parts of the CRA toolchain, since clearly building the app with webpack follows different compilation rules.

This is actually another subtlety to look out for with this PR. I had changed the imports to use ES6 modules because webpack actually looks for the module property in package.json and prefers ES6 modules, and since CPK defines that property, webpack actually resolves to the ES6 import path when importing contract-proxy-kit. Changing the provider import to use the ES6 version makes the imports more consistent on the webpack side, as well as enables more consistent code generation. In fact, ideally, since this frontend is in TypeScript, I would have opted for importing the TS paths (so everything under src instead of lib/esm or lib/cjs).

However, since Jest looks for CJS modules by default, when resolving paths, it doesn't do what webpack does, but instead uses the main property of the package.json in order to find a CJS module to resolve to. Since it doesn't understand the ES6 module import syntax, the test runner errors with the ES6 imports, but webpack is able to cope with both ES6 and CJS modules, and ES6 is really preferred on the webpack side because of tree-shaking.

Anyway, TL;DR: made tests pass again, but at a small cost to code compilation quality.

P.S. I've been trying to figure out the right config for getting both ES6 imports (or even TS imports) on the webpack side, but sticking to CJS on the Jest side, but it's been weird. There's no Babel config in this repo, which seems to be a prerequisite for setting up transformIgnorePatterns in the Jest configuration, according to some comments I found here. I think maybe CRA hides the Babel config in order to streamline development...?

@kadenzipfel
Copy link
Contributor

Tests work but I get this error when I run the app, which seems to be related to CPK.
image

Copy link
Contributor

@kadenzipfel kadenzipfel left a comment

Choose a reason for hiding this comment

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

See comment

@cag
Copy link
Contributor Author

cag commented Sep 29, 2020

Weird... try clearing your cache somehow? Like deleting ./node_modules/.cache and your build? I don't know why the deploy preview works still. Let me try yarn start on my machine though...

Edit: okay, it shows up in the deploy preview... :/

Edit 2: I guess the way to fix this would be to somehow fix how Jest loads the CPK, since build-wise, everything seemed to have worked when the ESM path was used...

@kadenzipfel
Copy link
Contributor

So I'm assuming we're better off going back to the ESM path and getting Jest to load the CPK properly as opposed to getting everything working using the CJS path?

The difficulty with this approach is that CRA doesn't let you modify the Jest config without ejecting. I wonder whether it would just be easier to get setup with an entirely new testing framework...

@cag
Copy link
Contributor Author

cag commented Sep 29, 2020

@kadenzipfel I found in the source of react-scripts that react-scripts test actually will spin up a Jest process with a configuration created on the fly, which explains why Jest is still able to read ESM from the app source even though it is loading modules in CJS mode. Also, found that the jest property in the appPackageJson can be used to override the configuration provided by react-scripts, so that's where transformIgnorePatterns would go. I've copied the transformIgnorePatterns found in react-scripts but carved out an exception for the contract-proxy-kit (this is what would also be used to support Jest tests for React Native).

@@ -1,4 +1,4 @@
import CPK from 'contract-proxy-kit'
import CPK from 'contract-proxy-kit/lib/esm'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing this to explicitly use the ESM path because Webpack and Jest resolves 'contract-proxy-kit' differently.

@kadenzipfel kadenzipfel merged commit 38f60d5 into protofire:master Sep 29, 2020
@pimato pimato added this to the Version 1.1.8 milestone Oct 8, 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.

3 participants