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

Update sample-rollup.config.js #1012

Closed
wants to merge 1 commit into from

Conversation

NullVoxPopuli
Copy link
Collaborator

Comment on lines 16 to 18
// This tells rollup to follow the node-resolution algorithm for finding
// dependencies / and files within those dependencies
nodeResolve({ extensions }),
Copy link
Collaborator

Choose a reason for hiding this comment

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

In general, I don't think we want folks to do this. This will make rollup embed your dependencies (not allow them to be resolved at application build time), which would be quite unfortunate. It would mean that the addon author in question would have to redeploy a new patch release for any security/patch releases in dependencies...

More details below in the externals section.

// This provides defaults that work well alongside `publicEntrypoints` below.
// You can augment this if you need to.
output: addon.output(),

// This prevents peerDependencies from being included in your bundle
external: Object.keys(packageJson['peerDependencies']),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is great! We need to beef it up though, it should also include:

  • The list of Ember's pseudo packages (e.g. @ember/object and friends). We should get that list from @embroider/shared-internals and expose it on the Addon instance we are using here.
  • The list of dependencies (the actual resolution / bundling will be done by the final app packager; either ember-auto-import@2 in classic builds or @embroider/webpack in Embroider builds)

import { Addon } from '@embroider/addon-dev/rollup';

import packageJson from './package.json';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, what makes this work? By default you can't import from .json files in Node...

Copy link
Contributor

Choose a reason for hiding this comment

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

Rollup configs go through their own weird preprocessing. That's also why they're written as ES modules despite predating Node's native ES module support.

const addon = new Addon({
srcDir: 'src',
destDir: 'dist',
});

const extensions = ['.js', '.ts'];
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we remove nodeResolve, I think we ought to be able move this to inline with babel with a nice little comment?

@ef4
Copy link
Contributor

ef4 commented Nov 4, 2021

I don't think we want nodeResolve in this default config. It will cause the dependencies to get copied into dist, which defeats yarn/npm deduplication and makes it impossible for consumers to benefit from semver-compatible releases of those dependencies.

Even in cases where rolling up a dependency is desired, it's only appropriate to do that for devDependencies, not dependencies, because otherwise consumers will end up with two copies.

So I think the real bug here is that we're letting rollup emit misleading warnings. The current output is correct, we just want to stop the warnings. I can whip up another little plugin to put in the default config that will achieve that.

@ef4
Copy link
Contributor

ef4 commented Nov 4, 2021

Here's how I would prefer to deal with the standard externals: #1015

@NullVoxPopuli
Copy link
Collaborator Author

legit, I'll rebase and update this once that's merged

@rwjblue
Copy link
Collaborator

rwjblue commented Nov 11, 2021

Should we close this now that #1015 landed?

@NullVoxPopuli
Copy link
Collaborator Author

I think the updates cover the dependency aspect of what I was trying to solve.

Should we still keep the extensions entry?

@ef4
Copy link
Contributor

ef4 commented Nov 12, 2021

I think this use of extensions is misleading. It makes it look like the config supports TypeScript, but it does not. For TS you still need to also add @babel/plugin-transform-typescript or @rollup/plugin-typescript. So I think we should leave it off.

We should probably be moving toward maintaining real blueprints rather than just this example file, in which case we could maintain a typescript blueprint that extends the base one, and that would be a good place to put all this.

@NullVoxPopuli
Copy link
Collaborator Author

Sounds good!

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