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

Support imports key from package.json (aka subpath imports) #2284

Open
chancancode opened this issue Feb 18, 2025 · 4 comments
Open

Support imports key from package.json (aka subpath imports) #2284

chancancode opened this issue Feb 18, 2025 · 4 comments

Comments

@chancancode
Copy link
Contributor

chancancode commented Feb 18, 2025

This is regarding this feature.

I couldn't find a consolidated place that documents the support status with subpath imports, so this documents my findings.

  • It is an emerging ecosystem standard. e.g. TypeScript supports with moduleResolution = bundler
  • There seems to be general consensus among the tooling team that this should work (correct me if I am work), and...
    • To the extent this kind of aliasing is necessary, then supporting a universal "standard" like this is better than supporting a matrix of various tool-specific configs
    • IMO this is becoming increasingly important with gjs/gts, where everything needs to be imported. For example, in a name-spaced app (e.g. @my-company/frontend) and components grouped in one or two levels of folders, your could be 4-5 slashes in before actually reaching the part of the path with useful signal.

Currently, in my experience:

  • TypeScript supports it...
    • ...but Glint 1 seems to not. However, it can be worked around by adding the aliases to tsconfig.compilerOptions.path.
  • Embroider does not support it...
    • At least with @embroider/compat/webpack, this fails with bug: embroider resolver's meta is not propagating
    • I haven't tried it with @embroider/vite – maybe it does work, but I am not so sure about that. The problem occurs inside @embroider/core, and to the extent that it still need to use the shared code to reason about the module graph at some point, it seems like things could subtly break.

In my case it seems like an acceptable workaround is (in a compat/webpack app) to use babel-plugin-module-resolver to rewrite the import paths before embroider sees it:

// ember-cli-build.js

'use strict';

const EmberApp = require('ember-cli/lib/broccoli/ember-app');

module.exports = async function (defaults) {
  const app = new EmberApp(defaults, {
    babel: {
      plugins: [
        // The `imports` key in package.json should be considered the source of
        // truth and this should be kept in-sync with that.
        [
          require.resolve('babel-plugin-module-resolver'),
          {
            extensions: ['.js', '.gjs', '.ts', '.gts'],
            alias: {
              '#': 'my-app', // imports: { "#/*": ["app/*"] }
              '#components': 'my-app/components', // imports: { "#components/*": ["app/components/*"] }
              '#tests': 'my-app/tests', // imports: { "#tests/*": ["tests/*"] }
            },
            // Suppresses warning: Could not resolve "my-app/..."
            loglevel: 'silent',
          },
        ],
      ],
    },
    // ...
  });
};

I would think this is preferable/more portable than trying to do similar things with webpack config. This will work with standard import statements and import().

A quick test seems to suggest that It but not the custom stuff like importSync() from @embroider/macros. I think that is because in a compat build, @embroider/macros inserts its babel transform at the beginning, so the importSync() is rewritten into require() before this babel-plugin-module-resolver sees it. That's probably fine and reliable enough.

I couldn't test import.meta.glob() because as far as I can tell it hasn't landed in the compat build yet. The babel plugin also has a transformFunctions option so maybe it can be configured to rewrite those as well if necessary. If babel-plugin-transform-vite-meta-glob is used, I suppose it would work too, as long as that is inserted before this plugin.

@NullVoxPopuli
Copy link
Collaborator

it works when you set type=module, see these examples:

afaik, imports only work with type=module

I have not tried setting type=module on a project with compat-ember

@chancancode
Copy link
Contributor Author

it works when you set type=module, see these examples:

afaik, imports only work with type=module

I have not tried setting type=module on a project with compat-ember

You mean with vite right? May want to edit your reply to be explicit about that because there are a few possible configurations here.

If we think the babel based workaround is good enough to recommend I think that seems fine to suggest doing that as a stepping stone before migrating to a vite build, then embroider wouldn’t have to implementing it specifically.

That being said, if we say this is a supported feature, I think it’s still suspicious that there is module resolution logic in @embroider/core that isn’t aware of subpath imports. It seems like things like @embroider/vite or the code mods stuff could easily accidentally use that logic and do the wrong thing.

@ef4
Copy link
Contributor

ef4 commented Feb 18, 2025

I think type="module" is a red herring here.

I just confirmed that in a stock @embroider/vite app subpath imports work out of the box for both the app's own code and for code in v2 addon dependencies.

On @embroider/webpack, the main thing that's going to get in your way is that you're still dependent on whole-app rewriting. The paths that you author are not the paths that actually exist by the time webpack is building your app. This is not a feature that can really be coherent while whole-app rewriting is happening. I expect an inverted-control webpack will also just work out of the box, the same way vite does.

So I don't think there's a todo here for Embroider. A critical thing about our architecture is that we delegate all module resolution to the host environment -- vite or webpack or (in the case of tools like codemods) node itself. So long as that environment supports subpath imports, so do we.

@mansona
Copy link
Member

mansona commented Feb 18, 2025

I think a key point here is that we need to mark this issue as "won't fix" for the current stable embroider. I think that's the confusion that we are facing right now 😞

For what it's worth I have updated the Vite app blueprint today to remove the warning about "don't use this ever for any app" 😂 https://github.com/embroider-build/app-blueprint?tab=readme-ov-file#embroiderapp-blueprint

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

No branches or pull requests

4 participants