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

Stabilize extensions for addon-dev's publicEntrypoints. #1223

Merged

Conversation

NullVoxPopuli
Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli commented Jun 28, 2022

Per some discussion in #1099
(and some common questions in discord),
we want to use .js for all file paths in the rollup config.

This PR transparently adds every extension (we only support 5) to each glob passed to addon.publicEntrypoints so that we can only specify .js, regardless of what the filetypes are on disk.

I'm currently getting stuck on this error:

(!) The emitted file "components/demo/index.js" overwrites a previously emitted file of the same name.

which you can reproduce via:

cd tests/scenarios
yarn test:output --scenario v2-addon-dev-typescript
cd output
cd node_modules/v2-addon
node node_modules/rollup/dist/bin/rollup -c ./rollup.config.mjs

@NullVoxPopuli NullVoxPopuli changed the title Stabilize extensions for publicEntrypoints. Stabilize extensions for addon-dev's publicEntrypoints. Jun 28, 2022
@ef4
Copy link
Contributor

ef4 commented Jun 28, 2022

This is the right goal but the wrong implementation. Rewriting people's globs is going to be fraught with unexpected behaviors. Lots of legal glob patterns won't have the file extension in the final position, or will have a false match for the extension in non-final positions.

Instead this should work the same way the existing hbs support works, which is:

  1. We run the users globs and a pattern for *.hbs.
  2. If we get a match against *.hbs we take that specific filename and replace the .hbs with .js, and then test whether that resulting name matches one of the user's patterns.

The reason this is better than rewriting the globs is that it's much easier to change the extension on an exact filename than on an arbitrary glob pattern. Just to give you one example to illustrate the problem, consider: 'ember.js/*/+(component.js|example.js)'.

@NullVoxPopuli
Copy link
Collaborator Author

NullVoxPopuli commented Jun 28, 2022

update -- I figured it out


where I'm failing is that I seem to somehow be removing ts files from the build entirely. they aren't transpiled by rollup-plugin-ts and the resulting ts+hbs co-located component ends up as a template-only component in dist

@NullVoxPopuli NullVoxPopuli force-pushed the addon-rollup-extension-stabilizing branch from c280f27 to c3bf61c Compare June 28, 2022 23:15
@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review June 28, 2022 23:15
);
let normalizedExists = pathExistsSync(id);

let matchesDeferred = [...args.include, '**/*.ts'].some(pattern => minimatch(name, pattern) )
Copy link
Collaborator Author

@NullVoxPopuli NullVoxPopuli Jun 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I'm least certain about :-\

It feels like a hack, and I don't know if it's correct.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if I only do:

            this.emitFile({
              type: 'chunk',
              id: join(args.srcDir, name),
              fileName: normalizedName,
            });

then we get:

(!) The emitted file "components/demo/index.js" overwrites a previously emitted file of the same name.

and the resulting component ends up being a template-only component -- ie, the class is lost

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would guess the TS plugin is already emitting that JS file. But it probably shouldn't be. Perhaps it has options to control that?

For example, it might be trying to emit a JS file for every TS file, but that would be wrong. We only want to emit public entrypoints, and let other modules get rolled up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it has options to control that?

plugins can't interact with other plugins tho? or can they?

maybe I don't understand?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean the options you're passing directly to the ts plugin in the example config.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are no options for passing entryPoints to the plugin? https://github.com/wessberg/rollup-plugin-ts

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NullVoxPopuli Hi, we use rollup-plugin-ts in a recent project and found it ditches the default types of generics in the output.

For example, for input like:

export interface Foo<T extends Bar | undefined = undefined> {
  ...
}

the output is:

export interface Foo <T extends Bar | undefined> {
  ...
}

So we end up to use rollup-plugin-typescript2 + rollup-plugin-dts instead. Just for your information, because this project is not an embroider project, we can not be sure if rollup-plugin-ts will be a problem here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think something else may have been at play in your project because rollup-plugin-ts very faithfully generates type definitions (I just confirmed just now by looking at the dists' d.ts files with default type args).

@NullVoxPopuli NullVoxPopuli force-pushed the addon-rollup-extension-stabilizing branch from fc89ea4 to cb00167 Compare July 7, 2022 21:00
@ef4 ef4 merged commit 0ce58c5 into embroider-build:main Jul 21, 2022
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