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] Add preserveModulesRoot config option #3786
Conversation
Thanks for the contribution, that is actually an interesting proposal. So if I understand this correctly what it does is:
then
So my initial thoughts were:
So if you would be willing to flesh out this PR, I would be willing to release it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides the comments that I left, these are the things that need to be done:
- Document the option in
docs/999-big-list-of-options.md
. I think it would be an "advanced" option. Mind the alphabetic ordering! It would also need to be added to the "Configuration Files" and "Command Line Flags" sections ofdocs/01-command-line-reference.md
, the "inputOptions object" section ofdocs/02-javascript-api.md
and thecli/help.md
file - Fix the existing tests. In this case, all you need to do is add the new option in the
test/misc/optionList.js
in the appropriate places. - Add a test. This one would go into the
chunking-form
category. If we allow both strings and regular expressions, then these would be two tests, one for each, and hopefully the regular expression test does something interesting that is not possible with strings...
src/Chunk.ts
Outdated
); | ||
if (options.moduleRootDir) { | ||
const moduleRootDir = resolve(options.moduleRootDir); | ||
const moduleRootDirRegExp = new RegExp(`^${moduleRootDir}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not quite happy about doing it this way. Just putting the string into a regular expression has some side-effects like .
matching any character that people may not be aware of. If we want to have the ability to have regular expressions, then I would prefer if we change the signature to string | RegExp
. If it is a string, we check via .startsWith
and slice the id, if it is a regular expression we replace. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review! Will start looking into these changes. With this one, I wasn't planning on using regex as an input option, so I think .startsWith
is definitely preferable
src/rollup/types.d.ts
Outdated
@@ -575,6 +575,7 @@ export interface OutputOptions { | |||
intro?: string | (() => string | Promise<string>); | |||
manualChunks?: ManualChunksOption; | |||
minifyInternalExports?: boolean; | |||
moduleRootDir?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the option only applies to the preserveModules case, I would suggest to change the name to reflect this like preserveModulesRoot
, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I like that name better
Also avoid regex, switch `path.resolve` to `path.join` due to lack of browser support for `path.join`
Codecov Report
@@ Coverage Diff @@
## master #3786 +/- ##
=======================================
Coverage 97.03% 97.03%
=======================================
Files 185 185
Lines 6467 6478 +11
Branches 1875 1877 +2
=======================================
+ Hits 6275 6286 +11
Misses 101 101
Partials 91 91
Continue to review full report at Codecov.
|
src/Chunk.ts
Outdated
); | ||
if (options.preserveModulesRoot) { | ||
const preserveModulesRoot = resolve(options.preserveModulesRoot); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is quite a bit of resolving
going on here which is not free as resolve
involves non-trivial file-system operations. So one way to get rid of the first one at least would be to perform this resolution once in normalizeOutputOptions
so that options.preserveModulesRoot
is already resolved at this point.
src/Chunk.ts
Outdated
} | ||
} | ||
const currentPath = `${currentDir}/${fileName}`; | ||
path = relative(preserveModulesRelativeDir, currentPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am thinking, is this really all necessary? Wouldn't this be the same:
if (currentDir.startsWith(preserveModulesRoot)) {
path = currentDir.slice(preserveModulesRoot.length).replace(/^\//, '')
} else {
path = relative(preserveModulesRelativeDir, `${currentDir}/${fileName}`);
}
or am I overlooking something here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In practice, this will probably will be the case, at least for right now. We would need to add a check somewhere to make sure preserveModulesRelativeDir
is resolved as a parent directory to preserveModulesRoot
for this to work out.
Edit: I think I figured this out -- this was close to what we needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot! I also very much like the test as you made sure that the ciritical edge cases of both node_modules and virtual modules are covered as well.
Looks like this is missing. |
Ups, thanks for noticing. I guess we can add this is an additional docs PR. As always if someone could put a few lines together in a separate docs PR, that would be awesome. Otherwise I can take a look tomorrow. |
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description
This PR adds a feature mentioned in #3785 that allows users to specify a
moduleRootDir
option. This allows users to specify how specific input modules' paths should be rendered in the output module. It is merely one implementation of multiple alternatives, so I figured I would draft up this PR as one resolution to #3785, but there may be better solutions for this.