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

@embroider/macros and type=module #1906

Draft
wants to merge 10 commits into
base: stable
Choose a base branch
from

Conversation

NullVoxPopuli
Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli commented May 7, 2024

Extracted from #1572 (which has more issues to fix (wrong css path, for example)

Two fixes here (specifically for type=module):

  • es-compat2 module was not resolvable (needed the js extensions)
  • importSync doesn't work with relative imports

@NullVoxPopuli NullVoxPopuli changed the base branch from main to stable May 7, 2024 14:08
@NullVoxPopuli NullVoxPopuli marked this pull request as draft May 7, 2024 14:08
Addon-main is cjs, it should have cjs extension

Reproduction success

Make separate scenario

Test file must end with -test

Need to update the app's ember-cli-babel

Test is passing, now let's break it again...

Break successful
@NullVoxPopuli NullVoxPopuli force-pushed the v2-addon-type-module-macros-resolve-build branch from 6ee93dd to 1439c04 Compare May 7, 2024 21:46
@NullVoxPopuli NullVoxPopuli force-pushed the v2-addon-type-module-macros-resolve-build branch from 59b4fe7 to 09dbe55 Compare May 7, 2024 21:48
@NullVoxPopuli
Copy link
Collaborator Author

NullVoxPopuli commented May 7, 2024

According to:

require() of ES Module /tmp/tmp-31383035liZVSdehi7p/node_modules/v2-addon/addon-main.js 
  from <.pnpm>/ember-cli@5.8.1/node_modules/ember-cli/lib/models/package-info-cache/package-info.js 
  not supported.
addon-main.js is treated as an ES module file as it is a .js file 
  whose nearest parent package.json contains "type": "module" which declares all .js files 
  in that package scope as ES modules.
Instead rename addon-main.js to end in .cjs, change the requiring code to use dynamic 
  import() which is available in all CommonJS modules, or change "type": "module" to 
  "type": "commonjs" in /tmp/tmp-31383035liZVSdehi7p/node_modules/v2-addon/package.json 
  to treat all .js files as CommonJS (using .mjs for all ES modules instead).

It is required that addon-main.js be cjs.

I had reverted that change earlier to reduce diff, forgetting that .js is not allowed to be ambiguous with type=module (the ambiguity is relied on for current non-type=module v2 addons)

@NullVoxPopuli
Copy link
Collaborator Author

Current issue (and the one causing the two tests to hang):
image

Died on test #1: Could not find module `./side-effecting.js` imported from `(require)`

it seems that importSync isn't working on relative imports perhaps?

@NullVoxPopuli
Copy link
Collaborator Author

Each time I push, I'm going to be cancelling the workflow so as to not hog CI

@NullVoxPopuli NullVoxPopuli changed the title @embroider/macros es-compat module is resolvable from type=module addons @embroider/macros and type=module May 9, 2024
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.

None yet

1 participant