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

feat: support ESM subpath imports #7770

Merged
merged 2 commits into from
Mar 6, 2023
Merged

Conversation

fi3ework
Copy link
Member

Description

resolve #7385

Vite now uses https://github.com/lukeed/resolve.exports to determine the target file of exports filed. According to the node.js spec and this lukeed/resolve.exports#14. This PR made a little fork of resolve.exports based on https://gist.github.com/okikio/3f07571c7707dc6e6eb4906b951c6bc3 to implement resolve.imports. We can remove the code from Vite until resovle.exports supports imports filed.

Additional context

https://github.com/fi3ework/vite/blob/7acab6a9db3aecb7b11d955598c1b48d58f7956b/packages/vite/src/node/plugins/resolve.ts#L152-L154

This PR re-assign id at the beginning for imports, I'm not sure is it the best way to do this or we could add another plugin like pre-alias? 🤔

  1. The test cases referenced evanw/esbuild@62a3943

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@sapphi-red sapphi-red added the enhancement New feature or request label Apr 20, 2022
@wight554
Copy link

Any blockers for this one?

@shirotech
Copy link

Awesome job, been looking for a way to do this, hope will be merged soon :)

@seivan
Copy link

seivan commented Jul 6, 2022

Maybe this isn't the right place, but if you're using package.json (assuming I got it right) for storing the module maps, that means everything else would have to infer from it.

Wouldn't that be an issue, if you would have to repeat it again for TSConfig?

Edit: Also, amazing work 🎉

Copy link
Collaborator

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

it looks like this PR needs a rebase

@patak-dev patak-dev added p3-minor-bug An edge case that only affects very specific usage (priority) p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) and removed p3-minor-bug An edge case that only affects very specific usage (priority) labels Aug 10, 2022
@bluwy
Copy link
Member

bluwy commented Aug 21, 2022

Update: From the last team meeting, we discussed that this is a feature we want, but we would want to use a centralized standard package to resolve subpath imports instead of a fork. We're currently coordinating with @lukeed to get this done, and after that we can update this PR to use it.

@brupelo
Copy link

brupelo commented Sep 18, 2022

Hi guys, vite newcomer over here, first of all, let me tell you how amazing vite project it's, in our case we've been already suffering some issue with a large CRA app that has hundreds of components and I'd really like to give it a shot to vite eventually but there are few blockers I'd need to solve before reaching that point and one of them being subpath imports support by the bundler.

I've tried to see if subpath imports was already working with latest vite atm (vite@3.1.2) but it seems is not, then I just found this PR.

Attaching a little reproducer in case it may help somehow test_storybook_subpath_imports.zip

When uncommenting line 2 of

image

You should get a crash like

image

Also, I guess this feature should be able to handle properly subpath imports containing non-js/jsx code, right? ie: assets such as json, svg, css, ...

ie: import reactLogo from '#assets/react.svg'`

@fi3ework
Copy link
Member Author

fi3ework commented Dec 15, 2022

It seems like https://github.com/privatenumber/resolve-pkg-maps is an alternative to https://github.com/lukeed/resolve.exports. Maybe we could move to resolve-pkg-maps as resolve.exports hasn't been maintained for a while. There is some difference described in https://github.com/privatenumber/resolve-pkg-maps#how-is-it-different-from-resolveexports, not sure these will be the breaking change. 🤔

@patak-dev
Copy link
Member

@fi3ework I think it is a good idea. IIRC @aleclarson also started a fork of lukeed/resolve.exports. Alec, it would interesting to hear your opinion here about @privatenumber new library.

@aleclarson
Copy link
Member

aleclarson commented Dec 15, 2022

We already have #10504 and #10929 to integrate @alloc/resolve.exports with Vite. I think there's value in using a solution built with Vite in mind, instead of a general purpose solution. Also, I think merging my resolve.exports fork into the Vite org would be worth while, so we don't have to rely on a third party for bug fixes to this critical piece of Vite's path resolution going forward.

@lukeed
Copy link

lukeed commented Dec 15, 2022

I like how “critical” resolve.exports is to vite & how much chatter there has been about it here (and elsewhere) yet there are no PRs from the work done here to fix the few bugs & add the really easy imports support. Nor have there been any offers to fund its maintenance or continued support. Not sure if it’s because people continue to assume/expect me to be a full-time OSS maintainer for free or because they see an opportunity to fork/copy-edit a new variant to siphon some “popularity” … either way, OSS has become such a game and I’m not here for it. Vite/Vue is sitting on a pile of cash & should be setting a clear example, while it has the attention, for how an open source ecosystem can grow and thrive sustainably instead of relying on an exorbitant amount of free labor.

@yyx990803
Copy link
Member

Hey @lukeed, first of all, thank you for your work on resolve.exports. Please note that we would never consider it your responsibility to implement anything that Vite needs specifically. It is your package, we do not expect anything more than what you've already done.

On the other hand, I think your accusation of "relying on an exorbitant amount of free labor" is misdirected. Vite is not making "a pile of cash" by any means. Our sponsorships barely generate enough to cover the salary of an entry level engineer, and we try to disperse it among team members who make consistent, direct contribution to Vite itself. Also do note I actually work on OSS fulltime, sponsor a lot of other devs in the Vue / Vite ecosystem, and need to make a living. Funding dependencies will be nice and we may consider it in the future, but I don't think that should be considered a responsibility.

We used resolve.exports because it aligned with what Vite needed. I think a fork / replacement is considered mainly because (1) assumed maintenance inactivity on resolve.exports. and (2) the possibility of needing more, maybe Vite-specific logic in the future.

The part I think we should have done better here is better communication with you from the beginning. I also 100% agree that we should've submitted a PR with the desired feature before moving towards a fork. Would you be open to consider that if we did?

@aleclarson
Copy link
Member

I like how “critical” resolve.exports is to vite & how much chatter there has been about it here (and elsewhere) yet there are no PRs from the work done here to fix the few bugs & add the really easy imports support. Nor have there been any offers to fund its maintenance or continued support. Not sure if it’s because people continue to assume/expect me to be a full-time OSS maintainer for free or because they see an opportunity to fork/copy-edit a new variant to siphon some “popularity” … either way, OSS has become such a game and I’m not here for it. Vite/Vue is sitting on a pile of cash & should be setting a clear example, while it has the attention, for how an open source ecosystem can grow and thrive sustainably instead of relying on an exorbitant amount of free labor.

@lukeed I feel a need to defend myself here. My fork of resolve.exports was strictly out of necessity. I just don't have the time or desire to jump through the hoops needed to get a PR merged into your library. I mean no offense. It really doesn't matter whether your library, a fork of it, or another library entirely is used for imports/exports mapping. What matters is that Vite users get this feature as soon as possible. I've done as much as I can to provide for that within my means. Finally, might I ask that you refrain from posting criticism openly before reaching out in private.

@lukeed
Copy link

lukeed commented Dec 16, 2022

We all know it’s much easier to patch fixes than it is to set up a new repo with all new tests, docs, and comparisons. And there’s nothing Vite needs that the ”general public” doesn’t also need. So I don’t understand that argument.

@yyx990803 theres clearly been motions towards a desired outcome already. Not that it was intentional or that you had any part in it, but the work has already been done and I’m not motivated by others’ desire for exposure/popularity/downloads. I have real bills to pay now and so OSS dramatically fell off the wayside. Congrats to you & others who’ve made it financially feasible, but I’ve invested 60+ hour weeks into OSS for years now and can’t justify “exposure” and “networking opportunities” for the time spent any more. I don’t mean this to be a personal soap opera hour but I’ve given away $500k equivalence of consulting hours over the years for little return. I don’t care about resolve.exports but it seemed like a great opportunity to point out the issues in OSS that are/were present here & I feel like Vite is in a great place to start changing things. Myself removed.

@dominikg
Copy link
Contributor

It really doesn't matter whether your library, a fork of it, or another library entirely is used for imports/exports mapping. What matters is that Vite users get this feature as soon as possible.

Imho it does matter to the node/web ecosystem at large. Having multiple packages doing the same thing in often subtly different ways isn't great. Fragmentation and all. This would hurt vite in the long run too as it has to deal with the fallout of any incompatibilities that arise out of this mess.

Vite has been great at working together with some upstream packages it depends on, rollup being the most prominent example here. Extending that pattern to other packages would benefit everyone. Sharing the burden of maintaining, using the public outreach it has to match up people who want to help with projects that need help etc. magic-string has been another example at this.

So please lets do OSS cooperation instead of fragmenting things even more 🥺

@lukeed
Copy link

lukeed commented Jan 15, 2023

resolve.exports@next just went out for early preview. It includes "imports" support via an imports function. The resolve function still exists & acts the same, but it's actually a convenience helper that reroutes to r.imports() or r.exports() depending on the target specifier it received. You can look over the API docs for more info.

Planning to release 2.0 stable this Monday or Tuesday if all looks to be ok

@lukeed
Copy link

lukeed commented Jan 26, 2023

Sorry, forgot to come back here with an update – the 2.0 was released a day after my last comment.

@benmccann
Copy link
Collaborator

Thanks @lukeed! It looks like Vite was upgraded to 2.0 here: #11712. We'll need another PR to leverage the imports support

@lukeed
Copy link

lukeed commented Jan 27, 2023

Ah cool! :)

@fi3ework
Copy link
Member Author

Have updated this PR with resovled.exports 2.0. Looking for some code reviews. 👀

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

LGTM. Great tests!

Comment on lines +159 to +161
const pkgJsonPath = lookupFile(basedir, ['package.json'], {
pathOnly: true,
})
Copy link
Member

Choose a reason for hiding this comment

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

Note to self. exports only work for root package.json, but node decided that imports doesn't need to and works for the closest package.json, so this is fine. Stackblitz

@bluwy bluwy added this to the 4.2 milestone Feb 26, 2023
@patak-dev patak-dev merged commit cc92da9 into vitejs:main Mar 6, 2023
@fi3ework fi3ework deleted the feat-7385 branch March 6, 2023 15:49
@lehni
Copy link

lehni commented Mar 6, 2023

Amazing! thank you @fi3ework for the hard work on this!

@lehni
Copy link

lehni commented Mar 7, 2023

Having read through the PR finally, I just realized that a massive thank you needs to go to @lukeed. I recognize the burden of maintaining OSS, and I appreciate all the work you're putting in for the rest of us!

@lukeed
Copy link

lukeed commented Mar 7, 2023

Thanks :) big thanks to @fi3ework for the integration here & to @bluwy for the all-around encouragement/motivation

@tsujp
Copy link

tsujp commented Mar 11, 2023

Looks like this has been included in Vite 4.2.0-beta.1 however unless I am using this incorrectly this doesn't appear to work in-situ. It does resolve a subpath import correctly but it cannot be used with a real world codebase because import-analysis does not understand what to do with a subpath import when given it and so it resolves to null instead which returns:

PM [vite] Internal server error: Failed to resolve import "#src/routes/app_router" from "src/main.tsx". Does the file exist?
Plugin: vite:import-analysis
File: /path/to/codebase/apps/www/src/main.tsx:1:26
4  |  import { $$registry as _$$registry } from "solid-refresh";
5  |  const _REGISTRY = _$$registry();
6  |  import { AppRouter } from '#src/routes/app_router';
   |                             ^

Annotated with a bunch of console.log statements when trying to debug this I can see the following:

(1) asked to resolve: #src/routes/app_router
(2) problem is here: null
(3) asked to resolve: #src/routes/app_router resolved to: [ './src/routes/app_router.js' ]

(1) is from within vite:import-analysis right before it switches on if (!resolved).

console.log('asked to resolve:', url)
const resolved = await this.resolve(url, importerFile);
                if (!resolved) {

(2) is from within that if statement.

(3) is from within vite:resolve.

import-analysis calls some resolve method (I'm working with the post-bundled source here so there are umpteen definitions for that function) which doesn't know what to do hence the failure.

I'll try actually find this in the normal codebase before it's been bundled.

Minimum reproducible example here: https://stackblitz.com/edit/github-c6tkmr-1eubsp?file=src/index.tsx

@privatenumber
Copy link
Contributor

privatenumber commented Mar 11, 2023

Looks like it's because your import map entry is:

"#src/*": "./src/*.js"

but you're using a TypeScript import:

import { Foo } from '#src/foo';

to resolve ./src/foo.tsx with a .js extension.

Vite will need a TypeScript compliant resolver for this. I'm actually working on one for tsx because it has the same issues.

@tsujp
Copy link

tsujp commented Mar 11, 2023

Looks like it's because your import map entry is:

"#src/*": "./src/*.js"

but you're using a TypeScript import:

import { Foo } from '#src/foo';

to resolve ./src/foo.tsx with a .js extension.

AFAIK that is correct because Typescript does not rewrite import paths (even though they actually do but that's a whole different discussion and a contentious one within the Typescript repo). Specifically here they do not rewrite the extensions. Even without an import map the only way to use that component would be to import it with the .js extension even though on-disk it is a .tsx component because after compilation it will be a .js file and... Typescript does not rewrite extensions.

Vite will need a TypeScript compliant resolver for this. I'm actually working on one for tsx because it has the same issues.

Ah yes I hadn't considered that Typescript will be resolving the import map in a manner consistent unto itself given the whole import-path-rewriting and that Vite may not be aware of that. As much as things like Vite have 100% improved the JS ecosystem it's (said ecosystem) still such a pot of spaghetti.

What's strange (to me at least) is that adding a very simple alias to the Vite configuration like the one below fixes this. There's no explicit extension changing here, just a simple find-replace

resolve: {
   alias: [
      {
         find: /^#src/,
         replacement: './src',
      }
}

@CMCDragonkai
Copy link

Question as far as I can tell, this is not working atm for vite 5.2.11. I still had to provide a tsconfigPaths() plugin to my vite.config.ts file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Support ESM-style "imports" field aliases in package.json