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

[Feature] Add preserveModulesRoot config option #3786

Merged
merged 11 commits into from Sep 21, 2020

Conversation

davidroeca
Copy link
Contributor

@davidroeca davidroeca commented Sep 17, 2020

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

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.

@lukastaegert
Copy link
Member

Thanks for the contribution, that is actually an interesting proposal. So if I understand this correctly what it does is:
If

  • we are preserving modules and
  • an id starts with the "moduleRootDir" option value,

then

  • replace it with an empty string

So my initial thoughts were:

  • that is actually a very simple and non-breaking approach, nicely done!
  • Is it configurable enough, i.e. are there situations where I want more control? But on second thought I think it is, at least I could not think of scenarios where I want more control. And there is already a note to give full control over the path names when preserving modules in Rollup 3.

So if you would be willing to flesh out this PR, I would be willing to release it.

Copy link
Member

@lukastaegert lukastaegert left a 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 of docs/01-command-line-reference.md, the "inputOptions object" section of docs/02-javascript-api.md and the cli/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}`);
Copy link
Member

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?

Copy link
Contributor Author

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

@@ -575,6 +575,7 @@ export interface OutputOptions {
intro?: string | (() => string | Promise<string>);
manualChunks?: ManualChunksOption;
minifyInternalExports?: boolean;
moduleRootDir?: string;
Copy link
Member

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?

Copy link
Contributor Author

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`
@davidroeca davidroeca marked this pull request as ready for review September 18, 2020 15:04
@davidroeca davidroeca changed the title [RFC] [Feature] Add moduleRootDir config option [Feature] Add moduleRootDir config option Sep 18, 2020
@davidroeca davidroeca changed the title [Feature] Add moduleRootDir config option [Feature] Add preserveModulesRoot config option Sep 18, 2020
@codecov
Copy link

codecov bot commented Sep 18, 2020

Codecov Report

Merging #3786 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           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           
Impacted Files Coverage Δ
src/utils/options/mergeOptions.ts 100.00% <ø> (ø)
src/Chunk.ts 100.00% <100.00%> (ø)
src/utils/options/normalizeOutputOptions.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb4d3a7...1f6d7a9. Read the comment docs.

src/Chunk.ts Outdated
);
if (options.preserveModulesRoot) {
const preserveModulesRoot = resolve(options.preserveModulesRoot);
Copy link
Member

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);
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Member

@lukastaegert lukastaegert left a 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.

@dasa
Copy link

dasa commented Sep 21, 2020

  • Document the option in docs/999-big-list-of-options.md.

Looks like this is missing.

@lukastaegert
Copy link
Member

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.

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

Successfully merging this pull request may close these issues.

None yet

3 participants