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(vite): add tsconfig paths resolution plugin #17844

Merged

Conversation

barbados-clemens
Copy link
Contributor

@barbados-clemens barbados-clemens commented Jun 28, 2023

Current Behavior

vite-tsconfig-paths plugin isn't able to work with tmp tsconfigs to make incremental building work

Expected Behavior

nx supports incremental building with vite

Related Issue(s)

terminal output showing number of modules transformed being less when using incremental building

TODO:

  • validation across different vite setups with/without buildable libs
  • write e2e tests
  • handle TODOs
  • cleanup register work within executor to just generate tsconfigs

Fixes #

@vercel
Copy link

vercel bot commented Jun 28, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nx-dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 17, 2023 3:22pm

@barbados-clemens barbados-clemens self-assigned this Jun 28, 2023
@barbados-clemens barbados-clemens added the scope: bundlers Issues related to webpack, rollup label Jun 28, 2023
@nx-cloud
Copy link

nx-cloud bot commented Jun 28, 2023

☁️ Nx Cloud Report

CI is running/has finished running commands for commit ce7876d. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@barbados-clemens barbados-clemens force-pushed the feat/vite-ts-paths-plugin branch from 948e075 to e698e4c Compare June 28, 2023 21:04
@barbados-clemens barbados-clemens force-pushed the feat/vite-ts-paths-plugin branch from e698e4c to 694577d Compare June 29, 2023 14:40
@barbados-clemens barbados-clemens force-pushed the feat/vite-ts-paths-plugin branch from 694577d to f9a319a Compare June 29, 2023 15:09
@barbados-clemens barbados-clemens force-pushed the feat/vite-ts-paths-plugin branch from f9a319a to 1a271ee Compare June 29, 2023 19:48
@barbados-clemens barbados-clemens force-pushed the feat/vite-ts-paths-plugin branch from 1a271ee to 12b029f Compare June 29, 2023 20:14
@barbados-clemens barbados-clemens force-pushed the feat/vite-ts-paths-plugin branch from 12b029f to d1849c8 Compare June 29, 2023 21:19
@barbados-clemens barbados-clemens force-pushed the feat/vite-ts-paths-plugin branch from d1849c8 to 9510617 Compare June 30, 2023 14:37
@barbados-clemens barbados-clemens force-pushed the feat/vite-ts-paths-plugin branch from 9510617 to 94ef222 Compare June 30, 2023 15:54
@barbados-clemens barbados-clemens marked this pull request as ready for review June 30, 2023 16:39
@barbados-clemens barbados-clemens requested a review from a team as a code owner June 30, 2023 16:39
@barbados-clemens barbados-clemens force-pushed the feat/vite-ts-paths-plugin branch from 94ef222 to 8d06002 Compare June 30, 2023 17:33
@barbados-clemens barbados-clemens force-pushed the feat/vite-ts-paths-plugin branch from 8d06002 to e931bdb Compare June 30, 2023 17:43
Comment on lines +56 to +61
try {
resolvedFile = matchTsPathEsm(source);
} catch (e) {
logIt('Using fallback path matching.');
resolvedFile = matchTsPathFallback?.(source);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This solves the issues I came across where a default @nx/js library will be built with the following package.json

{
  "name": "@acme/js-lib",
  "version": "0.0.1",
  "type": "module",
  "peerDependencies": {
    "tslib": "2.6.0"
  },
  "main": "./src/index.js",
  "types": "./src/index.d.ts"
}

and vite will be unable to load it as RollUp claims it cannot find the named exports, or the tsconfig-paths will not be able to resolve the exports object since it doesn't exist.

The same issue happens in the original plugin being used as well vite-tsconfig-paths.

This fallbacks to the root level tsconfig where the .ts files will be loaded instead so projects that don't include export objects or commonjs will still be able to be used in the project with buildLibsFromSource=false just will be forced to be build from source.

This can be fixed in the userland by changing the tsconfig settings to use NodeNext and type:module in package.json which allows vite to see the named exports

i.e. the output switches from

"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
const tslib_1 = require("tslib");
tslib_1.__exportStar(require("./lib/lib-js"), exports);
//# sourceMappingURL=index.js.map

to just

export * from './lib/lib-js';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jaysoo would like your opinion on this, to see if this makes sense or if we should see what we can do to get cjs built libs working inside vite.

Comment on lines -53 to -55
registerTsConfigPaths(tmpTsConfig);
} else {
registerTsConfigPaths(tsConfig);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can tell, since these paths never being used by vite downstream, there isn't a reason to register them anymore.

Copy link
Member

@mandarini mandarini left a comment

Choose a reason for hiding this comment

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

Interesting, and thank you @barbados-clemens !! I'm going to test this out today. I am thinking, should we really drop the use of vite-tsconfig-paths plugin, and rely only on the tsconfig-paths? I'm thinking that maybe vite-tsconfig-paths covers some specific use cases for vite, and using vite-tsconfig-paths in the nx-vite-paths plugin (instead of tsconfig-paths) could potentially save us some maintenance? Again, have not tried out the PR yet, will do today, and not sure what more vite-tsconfig-paths does, so I'm just speculating!

In any case, I'll test it out today! :D thanks!

I also want to see @jaysoo opinion!

@mandarini mandarini requested a review from jaysoo July 3, 2023 09:16
@mandarini
Copy link
Member

Cool! So, yes, I tested on this repo: https://github.com/mandarini/vite-paths/tree/test-caleb on the branch test-caleb, which I added lots of interdependencies, and some to cover Storybook path resolution as well. All works great! So it's a ✅ from me, but with the question whether we really want to drop vite-tsconfig-paths completely, and not rely on it at all!

Copy link
Member

@mandarini mandarini left a comment

Choose a reason for hiding this comment

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

Approving to move this forward, but still would like one look from @jaysoo

@barbados-clemens barbados-clemens force-pushed the feat/vite-ts-paths-plugin branch from e0eafcf to ce7876d Compare July 17, 2023 15:20
@barbados-clemens barbados-clemens merged commit 6529be9 into nrwl:master Jul 17, 2023
@barbados-clemens barbados-clemens deleted the feat/vite-ts-paths-plugin branch July 17, 2023 16:01
@github-actions
Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
scope: bundlers Issues related to webpack, rollup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

buildLibsFromSource: false doesn't work with Vite executors
2 participants