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 #1050 #1083

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix #1050 #1083

wants to merge 1 commit into from

Conversation

marksmccann
Copy link

@marksmccann marksmccann commented Oct 13, 2024

Related issue: #1050

Summary of the changes

  • Removed the type="module" key from the package's package.json files
    👆 This is the primary change that resolved the issue. The following changes were downstream of this change. Once this change was made the import worked again as expected:
    Screenshot 2024-10-12 at 9 13 27 PM
  • Renamed the package's lib/*.cjs file to lib/*.js.
    👆 The use of the .cjs extension is no longer required because the module type is no longer explicitly defined. Consumer's bundlers will be able to resolve both the es and cjs modules appropriately with standard .js extensions now.
  • Renamed the package's rollup.config.js files to rollup.config.mjs
    👆 Because the module type is longer explicitly defined, CommonJS is the default node environment for each package. This means that we need a .mjs to use es modules in all of the local node scripts.
  • Removed the c8 node --test-reporter=spec --loader part of the core test script
    🚨 I was unable to get the existing tests to work with the current configuration/setup and the aforementioned changes. I found the the tsx command worked fine on its own. However, there was some issue loading tsx through node that was preventing it from resolving the modules properly. I looked into it and it turns out that --loader option is actually experimental and may be removed. In fact, there is an existing warning about it in the console when the tests are run:
    Screenshot 2024-10-12 at 9 08 15 PM
    Obviously, I do not expect you to accept these updates without code coverage of your tests. However, I think this might be a good opportunity to resolve both issues (removal of --loader and type="module"). I am confident that an alternative methods or tool would work. FWIW, I have had great success with Jest and Vitest. If it would be helpful, I'd even be willing to do some of the footwork.

@alexandersorokin
Copy link

This doesn't solve underlying issue.
See: #1050 (comment)

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