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

Test v2 addon as a type=module package #1572

Draft
wants to merge 1 commit into
base: stable
Choose a base branch
from

Conversation

NullVoxPopuli
Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli commented Aug 4, 2023

Ran in to an issue with type=module over here ember-primitives#33 - ci-logs
and wanted to see if it was reproducible in a stripped down environment (it is!)

@NullVoxPopuli NullVoxPopuli changed the title make addon a module Test v2 addon as a type=module package Aug 4, 2023
@ef4
Copy link
Contributor

ef4 commented Sep 12, 2023

I found an approach that fixes this particular case:

diff --git a/packages/webpack/src/webpack-resolver-plugin.ts b/packages/webpack/src/webpack-resolver-plugin.ts
index b832434d..c0286b94 100644
--- a/packages/webpack/src/webpack-resolver-plugin.ts
+++ b/packages/webpack/src/webpack-resolver-plugin.ts
@@ -165,6 +165,7 @@ class WebpackModuleRequest implements ModuleRequest {
     if (this.fromFile === newFromFile) {
       return this;
     } else {
+      delete (this.state as any).resolveOptions;
       this.state.contextInfo.issuer = newFromFile;
       this.state.context = dirname(newFromFile);
       return new WebpackModuleRequest(this.babelLoaderPrefix, this.appRoot, this.state) as this;

The reason our initial attempt to mutate this.state.resolveOptions.byDependency.esm.fullySpecified didn't work is that webpack is doing a === on the resolveOptions to reuse a cached resolver. The resolveOptions seem to be optional, so deleting them is one solution. It also works if you clone the options first to give them a different object identity.

Next step here would be to figure out what we need to do with resolveOptions to be correct in all the cases. Is leaving it off OK? Are we obligated to recreate it in cases where we're rehoming a request into a package that actually needs fullySpecified: true?

A related question is: can we stop mutating this.state when we adjust webpack requests and instead produce complete valid new states? This would mean changing all the places where we pass this.state to new WebpackModuleRequest to instead derive a new state object. This would also solve the problem because we wouldn't accidentally carry across something like this option that we don't want to keep in the new request. The challenge is making sure we include exactly what's needed.

@NullVoxPopuli
Copy link
Collaborator Author

looks like this is a problem with importSync

I can't reproduce the behavior in the tests correctly, but
this simple file causes an error:

import { importSync } from '@embroider/macros';

export const two = 2;

importSync('./violations.css');

without importSync, type=module appears to work ok.
the error:

odule not found: Error: Can't resolve '../../node_modules/.pnpm/@embroider+macros@1.13.3/node_modules/@embroider/macros/src/addon/es-compat2' in '/home/nvp/Development$TMPDIR/type-module-test/type-module-test/dist/index.js'
Did you mean 'es-compat2.js'?
BREAKING CHANGE: The request '../../node_modules/.pnpm/@embroider+macros@1.13.3/node_modules/@embroider/macros/src/addon/es-compat2' failed to resolve only because it was resolved as fully specified
(probably because the origin is strict EcmaScript Module, e. g. a module with javascript mimetype, a '*.mjs' file, or a '*.js' file where the package.json contains '"type": "module"').
The extension in the request is mandatory for it to be fully specified.
Add the extension to the request.

@NullVoxPopuli
Copy link
Collaborator Author

NullVoxPopuli commented Nov 19, 2023

Looks like there are a few things happening here -- gonna have separate issue for them all.

@NullVoxPopuli
Copy link
Collaborator Author

Info from @mansona,
we might need 9e7d875 or browserify/resolve#320 to support it

@mansona
Copy link
Member

mansona commented Dec 13, 2023

@NullVoxPopuli is it worth trying again since #1686 was merged and we support the exports with require.resolve now?

@NullVoxPopuli
Copy link
Collaborator Author

I found that things work ok in the test environment without interacting with the @embroider/macros, but in the latest commit, #e7a924e, I added some macros usage to re-break the test.

Basic type=module support (ie #1674) now works, but it seems we have some compat stuff to do

@NullVoxPopuli NullVoxPopuli mentioned this pull request Jan 30, 2024
21 tasks
@NullVoxPopuli NullVoxPopuli changed the base branch from main to stable May 7, 2024 13:15
@NullVoxPopuli NullVoxPopuli changed the base branch from stable to main May 7, 2024 13:15
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 changed the base branch from main to stable May 7, 2024 13:16
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.

3 participants