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: require regular version in stubs #20

Merged
merged 2 commits into from
Nov 24, 2021

Conversation

achingbrain
Copy link
Collaborator

@achingbrain achingbrain commented Nov 23, 2021

When we stub files for environments that do not handle export maps very well, we require a transpiled CJS implementation and re-export it in a stub file that should be immediately overriden by the browser or exports fields in package.json.

At the moment we use the value from the browser overrides to work out which file we should require the stub. The change here is to require the transpiled regular CJS version in the stub instead of the browser override.

Turns out when Jest has testEnvironment: 'node' set it resolves requires to the stubs, ignoring exports which loads browser-specific code into node with predictably incendiary results.

When we stub files for environments that do not handle export maps
very well, we `require` a transpiled CJS implementation and re-export
it in a stub file that should be immediately overriden by the `browser`
or `exports` fields in `package.json`.

At the moment we use the value from the `browser` overrides to work out
which file we should `require` the stub. The change here is to `require`
the transpiled regular CJS version in the stub instead of the browser
override.

Turns out when Jest has `testEnvironment: 'node'` set it resolves `require`s
to the stubs, ignoreing `exports` which loads browser-specific code into
node with predictably incendiary results.
@achingbrain
Copy link
Collaborator Author

Fixes: ipfs/js-ipfs#3938

Copy link
Collaborator

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

just that one nit, otherwise I think this should be doing the right thing, letting the browser field do the work everywhere it's needed rather than baking that into the redirects


return stubs
}

async stubFiles (dist, overrides) {
if (typeof overrides === 'string') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

these 3 lines can be removed now

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

@achingbrain achingbrain merged commit 3b72280 into master Nov 24, 2021
@achingbrain achingbrain deleted the fix/require-regular-version-in-stubs branch November 24, 2021 07:29
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