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

Feature: allowAppImports #587

Merged
merged 16 commits into from
Nov 14, 2023
Merged

Feature: allowAppImports #587

merged 16 commits into from
Nov 14, 2023

Conversation

mansona
Copy link
Member

@mansona mansona commented Jul 13, 2023

This is taking over from #586 👍

Fixes #565
Fixes #158

@ef4
Copy link
Collaborator

ef4 commented Jul 13, 2023

TODO list items:

  • filtering matching files out of classic build pipeline. May involve monkey-patching the appInstance's trees.app.
  • teaching the splitter to emit imports for package-name-prefixed imports that match, like "my-app/lib/example1".
  • teaching the splitter to emit imports for relative imports for files that match, like "../lib/example1"
  • handling webpack externals so that when an auto-imported module imports a classically-built app module it doesn't get duplicated inside webpack. This is an extension to what we already do to support v2 addons using ember-provided features.
  • make babel-loader config sensitive to the fact that app code gets the full app babel config (whereas dependencies get a streamlined, standardized config)
  • make sure that the dynamic imports work as well as the static-imports test scenario

@@ -481,6 +482,15 @@ export default class Package {
}
}

get allowAppImports(): string[] {
// only apps (not addons) are allowed to set this
Copy link
Contributor

Choose a reason for hiding this comment

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

I discussed this feature a while ago with @ef4 within CrowdStrike, and IIRC we came to the conclusion that also addons should be able to do this. Example use case would be to have a v1 addon import its own css-modules based CSS (to get rid of ember-css-modules), so the consuming app could already switch to optimized Embroider (which ember-css-modules does not support) without requiring all addons to migrate to v2.

FWIW, this (v1 addon support of this feature) is not anymore a priority for us, but maybe it's still good to allow this!?

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 a good point.

I want to use this feature to make a package like https://github.com/cardstack/glimmer-scoped-css work in classic builds, and for that story to be complete you'd want a v1 addon to be able to use it too.

@mansona mansona self-assigned this Oct 2, 2023
@ef4 ef4 merged commit 04d3452 into main Nov 14, 2023
119 checks passed
@mansona mansona added the enhancement New feature or request label Nov 24, 2023
This was referenced Nov 24, 2023
Comment on lines +84 to +88
let host: AppInstance & {
trees: {
app: Node;
};
} = _host as any;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you force the type, could this work/be considered:

Suggested change
let host: AppInstance & {
trees: {
app: Node;
};
} = _host as any;
let host = _host as AppInstance & {
trees: {
app: Node;
};
};

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.

Feature: allow ember-auto-import to own parts of of the app How to dynamically import local modules?
4 participants