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: esm output's extension to .mjs #1343

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

syi0808
Copy link

@syi0808 syi0808 commented Jul 11, 2024

What:
Why:
More Information: kentcdodds/kcd-scripts#244

How:
Change module field extension to .mjs and upgrade kcd-scripts after pr released.

screenshots

after this pr build output is below.

스크린샷 2024-07-12 오후 2 08 07

before change dist file and package.json

스크린샷 2024-07-12 오후 3 02 54

after change dist file and package.json

스크린샷 2024-07-12 오후 3 02 32

Checklist:

  • Documentation added to the
  • Tests
  • TypeScript definitions updated
  • Ready to be merged

Additional

I think according to https://github.com/kentcdodds/kcd-scripts/releases. We can skip breaking changes from 14, 15 versions. (v14 breaking change is about preact, v15 breaking change is about node v14)

Resolves: #1338 testing-library/user-event#1213

Copy link

codesandbox-ci bot commented Jul 11, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 631747f:

Sandbox Source
react-testing-library-examples Configuration

@syi0808
Copy link
Author

syi0808 commented Jul 13, 2024

I don't know why formatting failed in workflow. In local, It passed. And Changes is not related to formatting.

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Why:
More Information: kentcdodds/kcd-scripts#244

It's not clear from this issue why we need to do this. Do you have a minimal repro highlighting what this fixes?

Using .mjs instead of .js is not trivial since older build setups may stop recognizing these files as JS so there should be a justification for making that change.

@syi0808
Copy link
Author

syi0808 commented Jul 15, 2024

It's not clear from this issue why we need to do this. Do you have a minimal repro highlighting what this fixes?

Using .mjs instead of .js is not trivial since older build setups may stop recognizing these files as JS so there should be a justification for making that change.

To be honest, I can't logically explain why it's an issue. I just found out that if you have a .mjs extension or a package.json with type:module, this will fix it.

I checked by modifying node_modules myself, so I don't know how to share repro. Should I give it to you as a compressed file?

@syi0808
Copy link
Author

syi0808 commented Jul 16, 2024

Repro

Project

Archive.zip
After decompressing, run npm i

testing library attached debug log

@testing-library.zip
After decompressing, paste in node_modules

@syi0808
Copy link
Author

syi0808 commented Jul 16, 2024

And more detail. @eps1lon

Testing-library/user-event in ESM format was fetching testing-library/dom in CJS format. But testing-library/react before the extension changed was getting testing-library/dom in ESM format.
After changing the extension, both are importing testing-library/dom in CJS format.

I'm not sure why you're looking for testing-library/dom in CJS format, but I think the current implementation is meaningful in unifying the different ways of working between the two libraries. I would like to think that this is a matter of node.

Screenshot 2024-07-16 at 9 26 28 AM Screenshot 2024-07-16 at 9 27 06 AM

@syi0808 syi0808 requested a review from eps1lon July 26, 2024 13:10
@syi0808
Copy link
Author

syi0808 commented Aug 9, 2024

Can we check it? @eps1lon @MatanBobi

@millerized
Copy link

millerized commented Aug 15, 2024

Why:
More Information: kentcdodds/kcd-scripts#244

It's not clear from this issue why we need to do this. Do you have a minimal repro highlighting what this fixes?

Using .mjs instead of .js is not trivial since older build setups may stop recognizing these files as JS so there should be a justification for making that change.

Hi @eps1lon! I believe there could be an issue with how the modules are being loaded based on ESM vs. CJS semantics. As mentioned in #1338 using latest Vitest, RTL, and userEvent together is "broken" and displaying errors in the console. We believe this is due to the userEvent APIs not being patched by RTL at runtime. When we debug this we find that identical esm/cjs modules are loaded and the incorrect one is monkey patched by RTL and this is why we see "not wrapped in act()" warnings.

I'm not exactly sure why @syi0808's change seems to fix this but if I had to guess it's the way that modules are loaded in node/vitest vs jest and this change makes things less ambiguous for non-jest runners. 🤷‍♂️

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.

RTL + Vitest issue with patching asyncWrapper/eventWrapper (with act()) for userEvent APIs
3 participants