feat: Run external check in cli/run/loadConfigFile.ts as last in order to allow handling of e.g. workspace package imports in TS monorepos correctly#6038
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
cli/run/loadConfigFile.ts to handle e.g. workspace package imports in TS monoreposcli/run/loadConfigFile.ts to handle e.g. workspace package imports in TS monorepos
|
I had to force-push to my forked repo because apparently first commit accidentally had wrong author :) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6038 +/- ##
=======================================
Coverage 98.79% 98.79%
=======================================
Files 271 271
Lines 10620 10622 +2
Branches 2838 2840 +2
=======================================
+ Hits 10492 10494 +2
Misses 88 88
Partials 40 40 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lukastaegert
left a comment
There was a problem hiding this comment.
Thank you for taking action here! However, the longer I look at this, the more problems I see with that approach. You already discovered some:
- no plugin context
- no support for async handlers
But the bigger issue for me that it is really complex and basically needs to reimplement the plugin logic from Rollup core.
And above all, the resolveId hook from the TypeScript plugin IS already running in your cases. It just has lower precedence than the external check, or rather, the external check runs first.
So why not go into a different direction: Instead of using the external option in loadConfigFile.ts, add a plugin AFTER addPluginsFromCommandOption (and/or make this a plugin with order: "post") which marks ids as external using the same logic that is currently used in the external check? That would be simple, solve all issues, and not require another config option, wouldn't it?
|
Thank you for taking time to review this and provide suggestions! 🙌 The suggestion by @lukastaegert makes quite a bit of sense. I will experiment locally and see if with that approach, the workspace references used within |
…ch simulates situation where rollup.config.ts imports package which looks like external, but is actually part of workspace. The test fails with expected error message now.
… config files, move externality check to be done as fallback instead of primary check.
|
I think I got it now. All tests, including added one, pass locally now, except some flaky one, which passes when run separately via |
cli/run/loadConfigFile.ts to handle e.g. workspace package imports in TS monoreposcli/run/loadConfigFile.ts as last in order to allow handling of e.g. workspace package imports in TS monorepos correctly
|
I noticed a few redundant parts in the test and simplified them directly in the PR. After submitting this commit, I realized that the test in this PR could be simplified. It might be enough to just verify that the TypeScript files in |
|
@TrickyPi All right, fair enough! It's true that it had this complex case of symlinking and such - I was trying to recreate the situation which produced the original problems as closely as possible within the test. Looks like current test passes with changes to Can't wait for the Rollup release after this gets merged, it will make a lot of plumbing code at the project I am working at simply disappear. 🙌 |
|
Hmm, maybe keeping the symbolic links that exist in a real workspace is better, since that’s what this PR wants to fix. |
lukastaegert
left a comment
There was a problem hiding this comment.
Looks good to me, thanks for taking the time, this will probably benefit many others!
|
That |
|
Yes, this is sometimes flaky. I will need to find a way to stabilize it. In the meantime, I will just retry |
|
This PR has been released as part of rollup@4.49.0. You can test it via |
This PR contains:
Are tests included?
The first commit adds a test to
test/cli/samples/config-ts-with-workspace-refs, which failed with the unmodified Rollup code.The second commit included changes to the
cli/run/loadConfigFile.tswhich made the test pass.Breaking Changes?
List any relevant issue numbers:
Resolves #6037 .
Description
resolveIdof additional plugin added after all plugins from commandline have been added.In case of TS config files (probably vast majority of usecase of
--configPlugin), this will cause workspace imports to be correctly resolved to internal dependencies.