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

Allow for more flexible addon-dev appReexports #1642

Merged

Conversation

krasnoukhov
Copy link
Contributor

While appReexports is pretty great at what it's doing it's not very flexible when you don't just export { default }. For example, if you have an initializer in v2 addon and you want to make it available to app without any additional config. But initializers require initialize function to be exported too. In our case some modules need to have export * as well.

This change creates a way to have support for such scenarios:

  • addon.appReexports can be called multiple times in the rollup config (we reset app-js initially when addon instance is created so there's no leftover stuff in there)
  • Optionally, for a specific glob, defaultExport can be also passed in which can be set to * or { default, initialize }

Tests were adjusted to account for all this. Let me know how this looks, any guidance will be appreciated. Cheers

Copy link
Contributor

@ef4 ef4 left a comment

Choose a reason for hiding this comment

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

Thanks, I hadn't noticed that initializers need this, but I see you are correct.

(It's arguably a mistake that initializers ever worked this way, given how everything else in the traditional system works, but that ship has sailed and this is a feature for achieving compatibility, so I agree we should support it.)

My suggestion for how to address the concerns I've pointed out here is an API like:

function appReexports(opts: {
  from: string;
  to: string;
  include: string[];
  mapFilename?: (filename: string) => string;
+ exports?: (filename: string) => string[] | undefined;
}) {}

Returning undefined from exports would be treated the same as returning ['default'].

A usage for initializers would look like:

addon.appReexports(['**/*.js'], {
  exports(filename) {
    if (/\/initializers\//.test(filename)) {
       return ['default', 'initialize'];
    }
  }
})

packages/addon-dev/src/rollup-app-reexports.ts Outdated Show resolved Hide resolved
packages/addon-dev/src/rollup-app-reexports.ts Outdated Show resolved Hide resolved
packages/addon-dev/src/rollup.ts Outdated Show resolved Hide resolved
packages/addon-dev/src/rollup.ts Outdated Show resolved Hide resolved
packages/addon-dev/src/rollup-app-reexports.ts Outdated Show resolved Hide resolved
@krasnoukhov
Copy link
Contributor Author

Thanks @ef4, good feedback, everything is addressed. One note: I changed function signature to string[] | string | undefined in order to also allow export * syntax. This is also covered by the test. I hope this is not a problem, but let me know. Cheers

@krasnoukhov
Copy link
Contributor Author

@ef4 Any other feedback here?

@NullVoxPopuli NullVoxPopuli added the enhancement New feature or request label Dec 19, 2023
@NullVoxPopuli NullVoxPopuli merged commit 2a6775b into embroider-build:main Dec 19, 2023
210 checks passed
@github-actions github-actions bot mentioned this pull request Dec 19, 2023
@krasnoukhov krasnoukhov deleted the addon-dev-reexports branch December 20, 2023 08:59
NullVoxPopuli added a commit that referenced this pull request Feb 1, 2024
Backport #1642 to stable:  Allow for more flexible addon-dev appReexports
@github-actions github-actions bot mentioned this pull request Feb 1, 2024
@github-actions github-actions bot mentioned this pull request Apr 4, 2024
@github-actions github-actions bot mentioned this pull request Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants