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: work with jest #14

Merged
merged 1 commit into from
Jul 15, 2021
Merged

Conversation

achingbrain
Copy link
Collaborator

Jest doesn't support package exports, nor does it support browser overrides out of the box (though it can be configured).

This means it parses the stubbed files introduced in #13 as javascript, so let's just require and export the file that the stub is stubbing.

This has the added bonus of also supporting old nodes that don't understand package exports.

Fixes achingbrain/uint8arrays#21

Jest [doesn't support package exports](jestjs/jest#9771),
nor does it support `browser` overrides out of the box (though it
[can be configured](https://jestjs.io/docs/configuration#resolver-string)).

This means it parses the stubbed files introduced in mikeal#13 as javascript,
so let's just require and export the file that the stub is stubbing.

This has the added bonus of also supporting old nodes that don't
understand package exports.

Fixes achingbrain/uint8arrays#21
@rvagg
Copy link
Collaborator

rvagg commented Jul 16, 2021

yeesh, this is a nasty fix; I'm getting a file without an extension now for every named export and the index.js is screwing up type generation inside the dist/ directory which I have for a lot of packages so I'm having to add it to index.js.

I'd like to flag this for reversion as soon as we get movement on Jest. I don't think they can hold out for too much longer as it's getting increasingly painful for their users. There are workarounds (and my opinion on this issue is that it's up to users to deal with it and put pressure on Jest, not us to pander to this weird environment) so this isn't unworkable for them. But there's also zero movement on browser-resolve, it's complicated and Jordan doesn't seem to have time to implement changes there. I think Jest is going to end up switching resolving engines because of this. Once that happens, I'd like to yeet this change, it's really unpleasant.

@rvagg
Copy link
Collaborator

rvagg commented Jul 16, 2021

Example of what I'm having to do to fix the breakage that flows from this change: ipld/js-car#39

For other packages where I'm actually using an index.js I'll have to rename that too.

@rvagg
Copy link
Collaborator

rvagg commented Jul 16, 2021

btw this is the solution I've been giving to people, which I implemented for js-dag-jose:
ceramicnetwork/js-dag-jose@51750b4#diff-3f698d0dc0e17487612dbe228105aa820683a2eb38343929c1c45d9a8aa479f8

I wrote this up in the Jest thread tracking the problem: jestjs/jest#9771 (comment) and other people have iterated on it for other usecases further down in that thread. From the volume of traffic in that thread I suspect this is going to slowly become a defacto for Jest setups.

achingbrain added a commit to ipld/js-car that referenced this pull request Jul 16, 2021
This module copies everything into the `dist` folder then builds types
from there.  `multiformats` builds types from the source, then copies
moves the `types` folder into `dist`.  The latter seems more predictable
and means things like mikeal/ipjs#14 don't
trip it up.

Also, doing `cp -a ... test/ dist/` instead of `cp -a ... test dist/`
meant that the contents of the test folder was copied into `dist` rather
than the test folder itself, which I guess was not intentional?  Removing
the trailing slash means you don't need the extra `cp` command to copy the
examples folder afterwards.
achingbrain added a commit to ipld/js-car that referenced this pull request Jul 16, 2021
This module copies everything into the `dist` folder then builds types
from there.  `multiformats` builds types from the source, then copies
moves the `types` folder into `dist`.  The latter seems more predictable
and means things like mikeal/ipjs#14 don't
trip it up.

Also, doing `cp -a ... test/ dist/` instead of `cp -a ... test dist/`
meant that the contents of the test folder was copied into `dist` rather
than the test folder itself, which I guess was not intentional?  Removing
the trailing slash means you don't need the extra `cp` command to copy the
examples folder afterwards.
achingbrain added a commit to ipld/js-car that referenced this pull request Jul 16, 2021
This module copies everything into the `dist` folder then builds types
from there.  `multiformats` builds types from the source, then copies
moves the `types` folder into `dist`.  The latter seems more predictable
and means things like mikeal/ipjs#14 don't
trip it up.

Also, doing `cp -a ... test/ dist/` instead of `cp -a ... test dist/`
meant that the contents of the test folder was copied into `dist` rather
than the test folder itself, which I guess was not intentional?  Removing
the trailing slash means you don't need the extra `cp` command to copy the
examples folder afterwards.
achingbrain added a commit to ipld/js-car that referenced this pull request Jul 16, 2021
This module copies everything into the `dist` folder then builds types
from there.  `multiformats` builds types from the source, then copies
moves the `types` folder into `dist`.  The latter seems more predictable
and means things like mikeal/ipjs#14 don't
trip it up.
@achingbrain
Copy link
Collaborator Author

achingbrain commented Jul 16, 2021

Ah, that's interesting, @ipld/js-car copies everything to the dist folder and builds types from the generated sources. multiformats takes a different approach, it builds types from the src dir, then copies the types into the dist folder. I think probably better not to build types from generated sources - I've opened ipld/js-car#40 which should sort that out.

@achingbrain
Copy link
Collaborator Author

Also, yeah, it would be better if jest supported exports and this was reverted, though best if browserify-resolve supported exports and #13 was reverted too as that would solve the problem at the root without everyone needing to maintain extra config.

It looks like Jordan wanted some help fleshing out https://github.com/ljharb/list-exports as a test case?

rvagg pushed a commit to ipld/js-car that referenced this pull request Jul 28, 2021
This module copies everything into the `dist` folder then builds types
from there.  `multiformats` builds types from the source, then copies
moves the `types` folder into `dist`.  The latter seems more predictable
and means things like mikeal/ipjs#14 don't
trip it up.
vasco-santos pushed a commit to vasco-santos/ipjs that referenced this pull request Jul 29, 2021
Jest [doesn't support package exports](jestjs/jest#9771),
nor does it support `browser` overrides out of the box (though it
[can be configured](https://jestjs.io/docs/configuration#resolver-string)).

This means it parses the stubbed files introduced in mikeal#13 as javascript,
so let's just require and export the file that the stub is stubbing.

This has the added bonus of also supporting old nodes that don't
understand package exports.

Fixes achingbrain/uint8arrays#21
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.

Friendly note on unexpected side-effects of 2.1.5->2.1.6 transition
3 participants