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 compatibility #37

Closed
wants to merge 2 commits into from
Closed

fix: ESM compatibility #37

wants to merge 2 commits into from

Conversation

ffMathy
Copy link

@ffMathy ffMathy commented Jan 29, 2024

Supersedes ftrackhq/ftrack-javascript#199

  • 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 29, 2024 10:24
@ffMathy ffMathy marked this pull request as draft January 29, 2024 10:24
@ffMathy ffMathy marked this pull request as ready for review January 29, 2024 10:25
@ffMathy
Copy link
Author

ffMathy commented Jan 29, 2024

CC @jimmycallin.

@jimmycallin
Copy link
Contributor

When you tested this by changing this locally, did you rename the outputted file to .mjs as well or just updated the package.json?

@ffMathy
Copy link
Author

ffMathy commented Jan 29, 2024

I renamed the file as well. I also just now tried renaming it back, and then it doesn't work. So it is indeed needed.

@jimmycallin
Copy link
Contributor

I'm a bit confused why this is needed for @ftrack/web-widget and not @ftrack/api so wonder if there is another underlying issue with the config that renaming to mjs hides. Will try to have a closer look later today.

@ffMathy
Copy link
Author

ffMathy commented Jan 30, 2024

I mean, either way it's good practice to use the mjs format, and I can't see a scenario where it breaks anything.

Let me know what you find either way. It's very interesting.

@jimmycallin
Copy link
Contributor

@ffMathy the issue is actually a missing property "type": "module" in package.json, mind updating your pr and verifying that it works?

@ffMathy
Copy link
Author

ffMathy commented Feb 10, 2024

Well, now we are switching away from Jest as well and to Vitest, just like you 😆 I think we got inspired.

The VS Code Extension for Jest is a bit annoying.

So I won't have time to check this out anymore unfortunately.

@jimmycallin
Copy link
Contributor

Good choice :) I'll go ahead and close this PR and apply the fixes separately

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