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

@lukastaegert lukastaegert commented Sep 18, 2020

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

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...
);
if (options.moduleRootDir) {
const moduleRootDir = resolve(options.moduleRootDir);
const moduleRootDirRegExp = new RegExp(`^${moduleRootDir}`);

This comment has been minimized.

@lukastaegert

lukastaegert Sep 18, 2020
Member

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?

This comment has been minimized.

@davidroeca

davidroeca Sep 18, 2020
Author Contributor

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;

This comment has been minimized.

@lukastaegert

lukastaegert Sep 18, 2020
Member

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?

This comment has been minimized.

@davidroeca

davidroeca Sep 18, 2020
Author Contributor

👍 I like that name better

davidroeca added 4 commits Sep 18, 2020
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 Sep 18, 2020
@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 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.

@davidroeca davidroeca requested a review from lukastaegert Sep 18, 2020
);
if (options.preserveModulesRoot) {
const preserveModulesRoot = resolve(options.preserveModulesRoot);

This comment has been minimized.

@lukastaegert

lukastaegert Sep 19, 2020
Member

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.

}
}
const currentPath = `${currentDir}/${fileName}`;
path = relative(preserveModulesRelativeDir, currentPath);

This comment has been minimized.

@lukastaegert

lukastaegert Sep 19, 2020
Member

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?

This comment has been minimized.

@davidroeca

davidroeca Sep 20, 2020
Author Contributor

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

davidroeca added 2 commits Sep 20, 2020
@davidroeca davidroeca requested a review from lukastaegert Sep 20, 2020
Copy link
Member

@lukastaegert lukastaegert left a comment

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.

@lukastaegert lukastaegert merged commit f6b14ef into rollup:master Sep 21, 2020
6 checks passed
6 checks passed
ci/circleci: analysis Your tests passed on CircleCI!
Details
ci/circleci: node-v10-latest Your tests passed on CircleCI!
Details
ci/circleci: node-v12-latest Your tests passed on CircleCI!
Details
ci/circleci: node-v14-latest Your tests passed on CircleCI!
Details
codecov/patch Codecov Report
Details
codecov/project Codecov Report
Details
@dasa
Copy link

@dasa 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

@lukastaegert lukastaegert commented Sep 22, 2020

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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.