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: proper naming for esm #199

Closed
wants to merge 2 commits into from
Closed

fix: proper naming for esm #199

wants to merge 2 commits into from

Conversation

ffMathy
Copy link
Contributor

@ffMathy ffMathy commented Jan 28, 2024

This fixes a bug where node ESM code does not recognize it as ESM in Jest (and possibly ESM node applications too), because the file extension is not .mjs.

I see this has already been fixed for "cjs". This just does it for "mjs" as well. Confimed working.

  • I have added automatic tests where applicable
  • The PR title is suitable as a release note
  • The PR contains a description of what has been changed
  • The description contains manual test instructions

Changes

Test

@ffMathy ffMathy requested a review from a team as a code owner January 28, 2024 20:47
@jimmycallin
Copy link
Contributor

are you sure this is correct? the vite config specifies output for es modules as having .js
image

i believe this is rather an issue with jest, and their lacking support of es modules jestjs/jest#9430 this was the main driver for us to switch to vitest

@ffMathy
Copy link
Contributor Author

ffMathy commented Jan 28, 2024

What I did to verify was to change the dist file locally in my own node-modules of the ftrack widget to be called .mjs instead of es.js.

FtrackWidget was the only package not working out of approximately 1800 of our packages for ESM in Jest. It gave an error saying "export" was not defined.

We just followed https://jestjs.io/docs/ecmascript-modules

In there, as of Node 20, ESM is working just fine (although through an experimental flag).

Although because I only verified it in the dist folder directly by modifying that locally, I didn't catch the build config change needed to. I'll update that.

@ffMathy
Copy link
Contributor Author

ffMathy commented Jan 28, 2024

I update the config now. Thanks for catching that.

@ffMathy
Copy link
Contributor Author

ffMathy commented Jan 29, 2024

@[email protected]

Here's a patch file that made Jest work in our environment.

@jimmycallin
Copy link
Contributor

jimmycallin commented Jan 29, 2024

@[email protected]

Here's a patch file that made Jest work in our environment.

is there a reason you provide the PR for this repository and not https://github.com/ftrackhq/ftrack-web-widget ?

@ffMathy
Copy link
Contributor Author

ffMathy commented Jan 29, 2024

Ooooh, I got it mixed up - my bad. I thought this was a kind of monorepo containing both 🤦

Will make it over there and close this.

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