Skip to content

Commit

Permalink
Bug/issue 820 deeply relative ESM paths in node modules not working (#…
Browse files Browse the repository at this point in the history 
…832)

* WIP supporting deep import specifier nesting

* refactor walkPackageJson to avoid manual relative file handling

* refactor our final single dot path replaces

* tweaks based on validation testing

* refactor bare module check

* refactor walk module to read file and handle deeply relative paths for named exports

* apply relative path handling across all AST visitor handlers

* dont walk packages that already provide export maps

* dont walk packages that already provide export maps

* force ESM style path separators

* tracked TODO item
  • Loading branch information
@thescientist13
thescientist13 authored Mar 5, 2022
1 parent 5c2aea0 commit 7697047
Show file tree
Hide file tree
Showing 6 changed files with 71 additions and 43 deletions.
1 change: 1 addition & 0 deletions packages/cli/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
"@lion/calendar": "^0.16.7",
"@mapbox/rehype-prism": "^0.5.0",
"@material/mwc-button": "^0.25.2",
"@stencil/core": "^2.12.0",
"@types/trusted-types": "^2.0.2",
"lit": "^2.0.0",
"lit-redux-router": "~0.20.0",
Expand Down
80 changes: 45 additions & 35 deletions packages/cli/src/plugins/resource/plugin-node-modules.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,19 @@ const updateImportMap = (entry, entryPath) => {
entryPath = `${entryPath}.js`;
}

importMap[entry] = entryPath;
// handle WIn v Unix-style path separators and force to /
importMap[entry.replace(/\\/g, '/')] = entryPath.replace(/\\/g, '/');
};

// handle ESM paths that have varying levels of nesting, e.g. export * from '../../something.js'
// https://github.com/ProjectEvergreen/greenwood/issues/820
async function resolveRelativeSpecifier(specifier, modulePath, dependency) {
const absoluteNodeModulesLocation = await getNodeModulesLocationForPackage(dependency);

// handle WIn v Unix-style path separators and force to /
return `${dependency}${path.join(path.dirname(modulePath), specifier).replace(/\\/g, '/').replace(absoluteNodeModulesLocation.replace(/\\/g, '/', ''), '')}`;
}

const getPackageEntryPath = async (packageJson) => {
let entry = packageJson.exports
? Object.keys(packageJson.exports) // first favor export maps first
Expand All @@ -41,58 +51,64 @@ const getPackageEntryPath = async (packageJson) => {
return entry;
};

const walkModule = async (module, dependency) => {
walk.simple(acorn.parse(module, {
const walkModule = async (modulePath, dependency) => {
const moduleContents = fs.readFileSync(modulePath, 'utf-8');

walk.simple(acorn.parse(moduleContents, {
ecmaVersion: '2020',
sourceType: 'module'
}), {
async ImportDeclaration(node) {
let { value: sourceValue } = node.source;
const absoluteNodeModulesLocation = await getNodeModulesLocationForPackage(dependency);
const isBarePath = sourceValue.indexOf('http') !== 0 && sourceValue.charAt(0) !== '.' && sourceValue.charAt(0) !== path.sep;
const hasExtension = path.extname(sourceValue) !== '';

if (path.extname(sourceValue) === '' && sourceValue.indexOf('http') !== 0 && sourceValue.indexOf('./') < 0) {
if (isBarePath && !hasExtension) {
if (!importMap[sourceValue]) {
// found a _new_ bare import for ${sourceValue}
// we should add this to the importMap and walk its package.json for more transitive deps
updateImportMap(sourceValue, `/node_modules/${sourceValue}`);
}

await walkPackageJson(path.join(absoluteNodeModulesLocation, 'package.json'));
} else if (sourceValue.indexOf('./') < 0) {
// adding a relative import
} else if (isBarePath) {
updateImportMap(sourceValue, `/node_modules/${sourceValue}`);
} else {
// walk this module for all its dependencies
sourceValue = sourceValue.indexOf('.js') < 0
sourceValue = !hasExtension
? `${sourceValue}.js`
: sourceValue;

if (fs.existsSync(path.join(absoluteNodeModulesLocation, sourceValue))) {
const moduleContents = fs.readFileSync(path.join(absoluteNodeModulesLocation, sourceValue));
await walkModule(moduleContents, dependency);
updateImportMap(`${dependency}/${sourceValue.replace('./', '')}`, `/node_modules/${dependency}/${sourceValue.replace('./', '')}`);
const entry = `/node_modules/${await resolveRelativeSpecifier(sourceValue, modulePath, dependency)}`;
await walkModule(path.join(absoluteNodeModulesLocation, sourceValue), dependency);

updateImportMap(path.join(dependency, sourceValue), entry);
}
}

return Promise.resolve();
},
ExportNamedDeclaration(node) {
async ExportNamedDeclaration(node) {
const sourceValue = node && node.source ? node.source.value : '';

if (sourceValue !== '' && sourceValue.indexOf('http') !== 0) {
// handle relative specifier
if (sourceValue.indexOf('.') === 0) {
updateImportMap(`${dependency}/${sourceValue.replace('./', '')}`, `/node_modules/${dependency}/${sourceValue.replace('./', '')}`);
const entry = `/node_modules/${await resolveRelativeSpecifier(sourceValue, modulePath, dependency)}`;

updateImportMap(path.join(dependency, sourceValue), entry);
} else {
// handle bare specifier
updateImportMap(sourceValue, `/node_modules/${sourceValue}`);
}
}
},
ExportAllDeclaration(node) {
async ExportAllDeclaration(node) {
const sourceValue = node && node.source ? node.source.value : '';

if (sourceValue !== '' && sourceValue.indexOf('http') !== 0) {
if (sourceValue.indexOf('.') === 0) {
updateImportMap(`${dependency}/${sourceValue.replace('./', '')}`, `/node_modules/${dependency}/${sourceValue.replace('./', '')}`);
const entry = `/node_modules/${await resolveRelativeSpecifier(sourceValue, modulePath, dependency)}`;

updateImportMap(path.join(dependency, sourceValue), entry);
} else {
updateImportMap(sourceValue, `/node_modules/${sourceValue}`);
}
Expand All @@ -107,7 +123,7 @@ const walkPackageJson = async (packageJson = {}) => {
// and walk its package.json for its dependencies

for (const dependency of Object.keys(packageJson.dependencies || {})) {
const dependencyPackageRootPath = path.join(process.cwd(), './node_modules', dependency);
const dependencyPackageRootPath = path.join(process.cwd(), 'node_modules', dependency);
const dependencyPackageJsonPath = path.join(dependencyPackageRootPath, 'package.json');
const dependencyPackageJson = JSON.parse(fs.readFileSync(dependencyPackageJsonPath, 'utf-8'));
const entry = await getPackageEntryPath(dependencyPackageJson);
Expand Down Expand Up @@ -137,7 +153,7 @@ const walkPackageJson = async (packageJson = {}) => {
break;
case 'object':
const entryTypes = Object.keys(mapItem);

if (entryTypes.import) {
esmPath = entryTypes.import;
} else if (entryTypes.require) {
Expand Down Expand Up @@ -165,31 +181,26 @@ const walkPackageJson = async (packageJson = {}) => {

// use the dependency itself as an entry in the importMap
if (entry === '.') {
updateImportMap(dependency, `/node_modules/${dependency}/${packageExport.replace('./', '')}`);
updateImportMap(dependency, `/node_modules/${path.join(dependency, packageExport)}`);
}
} else if (exportMapEntry.endsWith && (exportMapEntry.endsWith('.js') || exportMapEntry.endsWith('.mjs')) && exportMapEntry.indexOf('*') < 0) {
// is probably a file, so _not_ an export array, package.json, or wildcard export
packageExport = exportMapEntry;
}

if (packageExport) {
const packageExportLocation = path.join(absoluteNodeModulesLocation, packageExport.replace('./', ''));
const packageExportLocation = path.resolve(absoluteNodeModulesLocation, packageExport);

// check all exports of an exportMap entry
// to make sure those deps get added to the importMap
if (packageExport.endsWith('js')) {
const moduleContents = fs.readFileSync(packageExportLocation);

await walkModule(moduleContents, dependency);
updateImportMap(`${dependency}${entry.replace('.', '')}`, `/node_modules/${dependency}/${packageExport.replace('./', '')}`);
updateImportMap(path.join(dependency, entry), `/node_modules/${path.join(dependency, packageExport)}`);
} else if (fs.lstatSync(packageExportLocation).isDirectory()) {
fs.readdirSync(packageExportLocation)
.filter(file => file.endsWith('.js') || file.endsWith('.mjs'))
.forEach((file) => {
updateImportMap(`${dependency}/${packageExport.replace('./', '')}${file}`, `/node_modules/${dependency}/${packageExport.replace('./', '')}${file}`);
updateImportMap(path.join(dependency, packageExport, file), `/node_modules/${path.join(dependency, packageExport, file)}`);
});
} else {
console.warn('Warning, not able to handle export', `${dependency}/${packageExport}`);
console.warn('Warning, not able to handle export', path.join(dependency, packageExport));
}
}
}
Expand All @@ -200,10 +211,9 @@ const walkPackageJson = async (packageJson = {}) => {

// sometimes a main file is actually just an empty string... :/
if (fs.existsSync(packageEntryPointPath)) {
const packageEntryModule = fs.readFileSync(packageEntryPointPath, 'utf-8');

await walkModule(packageEntryModule, dependency);
updateImportMap(dependency, `/node_modules/${dependency}/${entry}`);
updateImportMap(dependency, `/node_modules/${path.join(dependency, entry)}`);

await walkModule(packageEntryPointPath, dependency);
await walkPackageJson(dependencyPackageJson);
}
}
Expand Down
17 changes: 16 additions & 1 deletion packages/cli/test/cases/develop.default/develop.default.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,18 @@ describe('Develop Greenwood With: ', function() {
`${process.cwd()}/node_modules/tslib/*.js`,
`${outputPath}/node_modules/tslib/`
);
const stencilCorePackageJson = await getDependencyFiles(
`${process.cwd()}/node_modules/@stencil/core/package.json`,
`${outputPath}/node_modules/@stencil/core/`
);
const stencilCoreCoreLibs = await getDependencyFiles(
`${process.cwd()}/node_modules/@stencil/core/internal/stencil-core/*.js`,
`${outputPath}/node_modules/@stencil/core/internal/stencil-core/`
);
const stencilCoreClientLibs = await getDependencyFiles(
`${process.cwd()}/node_modules/@stencil/core/internal/client/*.js`,
`${outputPath}/node_modules/@stencil/core/internal/client/`
);

// manually copy all these @babel/runtime files recursively since there are too many of them to do it individually
const babelRuntimeLibs = await rreaddir(`${process.cwd()}/node_modules/@babel/runtime`);
Expand Down Expand Up @@ -437,7 +449,10 @@ describe('Develop Greenwood With: ', function() {
...materialThemePackageJson,
...materialThemeLibs,
...tslibPackageJson,
...tslibLibs
...tslibLibs,
...stencilCorePackageJson,
...stencilCoreCoreLibs,
...stencilCoreClientLibs
]);

return new Promise(async (resolve) => {
Expand Down
10 changes: 3 additions & 7 deletions packages/cli/test/cases/develop.default/import-map.snapshot.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,11 +166,8 @@
"@lion/button": "/node_modules/@lion/button/index.js",
"@lion/core": "/node_modules/@lion/core/index.js",
"@lion/core/differentKeyEventNamesShimIE": "/node_modules/@lion/core/src/differentKeyEventNamesShimIE.js",
"@lion/button/src/LionButton.js": "/node_modules/@lion/button/src/LionButton.js",
"@lion/button/define-button": "/node_modules/@lion/button/lion-button.js",
"@lion/button/src/LionButtonReset.js": "/node_modules/@lion/button/src/LionButtonReset.js",
"@lion/button/define-button-reset": "/node_modules/@lion/button/lion-button-reset.js",
"@lion/button/src/LionButtonSubmit.js": "/node_modules/@lion/button/src/LionButtonSubmit.js",
"@lion/button/define-button-submit": "/node_modules/@lion/button/lion-button-submit.js",
"@lion/button/define": "/node_modules/@lion/button/define.js",
"lit": "/node_modules/lit/index.js",
Expand Down Expand Up @@ -258,17 +255,16 @@
"lit-element/decorators/state.js": "/node_modules/lit-element/decorators/state.js",
"lit-element/polyfill-support.js": "/node_modules/lit-element/polyfill-support.js",
"lit-element/private-ssr-support.js": "/node_modules/lit-element/private-ssr-support.js",
"lit-html/lit-html.js": "/node_modules/lit-html/lit-html.js",
"lit-html/polyfill-support.js": "/node_modules/lit-html/polyfill-support.js",
"lit-html/private-ssr-support.js": "/node_modules/lit-html/private-ssr-support.js",
"@lion/calendar": "/node_modules/@lion/calendar/index.js",
"@lion/localize": "/node_modules/@lion/localize/index.js",
"@lion/calendar/src/LionCalendar.js": "/node_modules/@lion/calendar/src/LionCalendar.js",
"@lion/calendar/define": "/node_modules/@lion/calendar/lion-calendar.js",
"@lion/calendar/test-helpers": "/node_modules/@lion/calendar/test-helpers/index.js",
"@lion/localize/test-helpers": "/node_modules/@lion/localize/test-helpers/index.js",
"@bundled-es-modules/message-format/MessageFormat.js": "/node_modules/@bundled-es-modules/message-format/MessageFormat.js",
"@bundled-es-modules/message-format": "/node_modules/@bundled-es-modules/message-format/index.js",
"singleton-manager/src/SingletonManagerClass.js": "/node_modules/singleton-manager/src/SingletonManagerClass.js",
"singleton-manager": "/node_modules/singleton-manager/index.js"
"singleton-manager": "/node_modules/singleton-manager/index.js",
"@stencil/core": "/node_modules/@stencil/core/internal/stencil-core/index.js",
"@stencil/client/index.js": "/node_modules/@stencil/core/internal/client/index.js"
}
1 change: 1 addition & 0 deletions packages/cli/test/cases/develop.default/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
"@lion/button": "^0.14.3",
"@lion/calendar": "^0.16.5",
"@material/mwc-button": "^0.25.2",
"@stencil/core": "^2.12.0",
"@types/trusted-types": "^2.0.2",
"lit": "^2.0.0"
}
Expand Down
5 changes: 5 additions & 0 deletions yarn.lock
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2481,6 +2481,11 @@
resolved "https://registry.yarnpkg.com/@rollup/stream/-/stream-2.0.0.tgz#2ada818c2d042e37f63119d7bf8bbfc71792f641"
integrity sha512-HsCyY/phZMys1zFUYoYlnDJGG9zMmYFfEjDKNQa00CYgjeyGD4cLdO6KNIkBh61AWOZfOsTPuGtNmFCsjQOfFg==

"@stencil/core@^2.12.0":
version "2.12.0"
resolved "https://registry.yarnpkg.com/@stencil/core/-/core-2.12.0.tgz#5b12517dd367908026692d3b00fa1aab39638ab9"
integrity sha512-hQlQKh5CUJe8g3L5avLLsfgVu95HMS2LToTtS7gpvvP0eKes1VvAC56uhI+vH4u44GZl9ck/g1rJBVRmMWu0LA==

"@stylelint/postcss-css-in-js@^0.37.2":
version "0.37.2"
resolved "https://registry.yarnpkg.com/@stylelint/postcss-css-in-js/-/postcss-css-in-js-0.37.2.tgz#7e5a84ad181f4234a2480803422a47b8749af3d2"
Expand Down

0 comments on commit 7697047

Please sign in to comment.