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.

Sign up for GitHub

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

Jump to bottom

[Feature] Add preserveModulesRoot config option #3786

Merged
merged 11 commits into from Sep 21, 2020
Merged

[Feature] Add preserveModulesRoot config option #3786

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
ab49e9a
add initial moduleRootDir implementation
davidroeca Sep 17, 2020
bb3603a
moduleRootDir -> preserveModulesRoot
davidroeca Sep 18, 2020
45c7516
update cli help + docs
davidroeca Sep 18, 2020
b8e5e5a
fix optionList test
davidroeca Sep 18, 2020
c6628e5
add preserveModulesRoot test
davidroeca Sep 18, 2020
4b629a6
fix preserveModulesRootDir -> preserveModulesRoot
davidroeca Sep 18, 2020
ab4eb7b
Merge branch 'master' into module-root-dir
davidroeca Sep 18, 2020
181397a
fix preserveModulesRootDir -> preserveModulesRoot
davidroeca Sep 18, 2020
bb16a35
Fix cli help styling
davidroeca Sep 18, 2020
e536077
Refactor code
davidroeca Sep 20, 2020
1f6d7a9
early return -> path =
davidroeca Sep 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 24 additions & 14 deletions src/Chunk.ts
View file
Open in desktop
Expand Up @@ -49,7 +49,7 @@ import {
isDefaultAProperty,
namespaceInteropHelpersByInteropType
} from './utils/interopHelpers';
import { basename, dirname, extname, isAbsolute, normalize, resolve } from './utils/path';
import { basename, dirname, extname, isAbsolute, join, normalize, resolve } from './utils/path';
import { PluginDriver } from './utils/PluginDriver';
import relativeId, { getAliasName } from './utils/relativeId';
import renderChunk from './utils/renderChunk';
Expand Down Expand Up @@ -441,20 +441,30 @@ export default class Chunk {
? '[name].js'
: '[name][extname].js'
: options.entryFileNames;
path = relative(
preserveModulesRelativeDir,
`${dirname(sanitizedId)}/${renderNamePattern(
pattern,
'output.entryFileNames',
{
ext: () => extension.substr(1),
extname: () => extension,
format: () => options.format as string,
name: () => this.getChunkName()
},
this.getChunkInfo.bind(this)
)}`
let currentDir = dirname(sanitizedId);
const fileName = renderNamePattern(
pattern,
'output.entryFileNames',
{
ext: () => extension.substr(1),
extname: () => extension,
format: () => options.format as string,
name: () => this.getChunkName()
},
this.getChunkInfo.bind(this)
);
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

if (currentDir.match(moduleRootDirRegExp)) {
currentDir = join(
preserveModulesRelativeDir,
currentDir.replace(moduleRootDirRegExp, '')
);
}
}
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

} else {
path = `_virtual/${basename(sanitizedId)}`;
}
Expand Down
2 changes: 2 additions & 0 deletions src/rollup/types.d.ts
View file
Open in desktop
Expand Up @@ -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

name?: string;
namespaceToStringTag?: boolean;
noConflict?: boolean;
Expand Down Expand Up @@ -620,6 +621,7 @@ export interface NormalizedOutputOptions {
intro: () => string | Promise<string>;
manualChunks: ManualChunksOption;
minifyInternalExports: boolean;
moduleRootDir: string | undefined;
name: string | undefined;
namespaceToStringTag: boolean;
noConflict: boolean;
Expand Down
1 change: 1 addition & 0 deletions src/utils/options/mergeOptions.ts
View file
Open in desktop
Expand Up @@ -213,6 +213,7 @@ function mergeOutputOptions(
intro: getOption('intro'),
manualChunks: getOption('manualChunks'),
minifyInternalExports: getOption('minifyInternalExports'),
moduleRootDir: getOption('moduleRootDir'),
name: getOption('name'),
namespaceToStringTag: getOption('namespaceToStringTag'),
noConflict: getOption('noConflict'),
Expand Down
1 change: 1 addition & 0 deletions src/utils/options/normalizeOutputOptions.ts
View file
Open in desktop
Expand Up @@ -56,6 +56,7 @@ export function normalizeOutputOptions(
intro: getAddon(config, 'intro'),
manualChunks: getManualChunks(config, inlineDynamicImports, preserveModules, inputOptions),
minifyInternalExports: getMinifyInternalExports(config, format, compact),
moduleRootDir: config.moduleRootDir as string | undefined,
name: config.name as string | undefined,
namespaceToStringTag: (config.namespaceToStringTag as boolean | undefined) || false,
noConflict: (config.noConflict as boolean | undefined) || false,
Expand Down
2 changes: 1 addition & 1 deletion src/utils/path.ts
View file
Open in desktop
Expand Up @@ -14,4 +14,4 @@ export function normalize(path: string) {
return path.replace(/\\/g, '/');
}

export { basename, dirname, extname, relative, resolve } from 'path';
export { basename, dirname, extname, relative, resolve, join } from 'path';