From 0ba7584cace9f5cf8126e07ee66b923f4ca2d5ad Mon Sep 17 00:00:00 2001 From: Caleb Eby Date: Wed, 7 Jul 2021 15:48:39 -0700 Subject: [PATCH] Allow resolving subfolders with package.json files, and improve CJS interop (#143) --- .changeset/pink-dogs-accept.md | 5 + .changeset/young-dancers-promise.md | 5 + jest.config.js | 4 +- package-lock.json | 39 +++++- package.json | 3 +- src/module-server/plugins/npm-plugin.ts | 177 +++++++++++++++--------- tests/utils/runJS.test.tsx | 61 +++++--- 7 files changed, 200 insertions(+), 94 deletions(-) create mode 100644 .changeset/pink-dogs-accept.md create mode 100644 .changeset/young-dancers-promise.md diff --git a/.changeset/pink-dogs-accept.md b/.changeset/pink-dogs-accept.md new file mode 100644 index 00000000..0b3a75d0 --- /dev/null +++ b/.changeset/pink-dogs-accept.md @@ -0,0 +1,5 @@ +--- +'pleasantest': patch +--- + +Improve CJS interop with packages that can't be statically analyzed diff --git a/.changeset/young-dancers-promise.md b/.changeset/young-dancers-promise.md new file mode 100644 index 00000000..e0abf0f1 --- /dev/null +++ b/.changeset/young-dancers-promise.md @@ -0,0 +1,5 @@ +--- +'pleasantest': patch +--- + +Allow resolving subfolders with package.json files diff --git a/jest.config.js b/jest.config.js index 42257627..9518cd52 100644 --- a/jest.config.js +++ b/jest.config.js @@ -1,10 +1,10 @@ module.exports = { testEnvironment: 'node', moduleNameMapper: { - pleasantest: '/dist/cjs/index.cjs', + '^pleasantest$': '/dist/cjs/index.cjs', }, testRunner: 'jest-circus/runner', - watchPathIgnorePatterns: ['/src/'], + watchPathIgnorePatterns: ['/src/', '/.cache'], transform: { '^.+\\.[jt]sx?$': ['esbuild-jest', { sourcemap: true }], }, diff --git a/package-lock.json b/package-lock.json index e9019441..abf11728 100644 --- a/package-lock.json +++ b/package-lock.json @@ -51,6 +51,7 @@ "polka": "0.5.2", "preact": "10.5.14", "prettier": "2.3.2", + "prop-types": "^15.7.2", "react": "17.0.2", "react-dom": "17.0.2", "remark-cli": "9.0.0", @@ -66,7 +67,7 @@ "typescript": "4.3.5" }, "engines": { - "node": "12 || 14 || 16" + "node": "^12.2 || 14 || 16" } }, "node_modules/@ampproject/remapping": { @@ -17827,6 +17828,23 @@ "node": ">= 6" } }, + "node_modules/prop-types": { + "version": "15.7.2", + "resolved": "https://registry.npmjs.org/prop-types/-/prop-types-15.7.2.tgz", + "integrity": "sha512-8QQikdH7//R2vurIJSutZ1smHYTcLpRWEOlHnzcWHmBYrOGUysKwSsrC89BCiFj3CbrfJ/nXFdJepOVrY1GCHQ==", + "dev": true, + "dependencies": { + "loose-envify": "^1.4.0", + "object-assign": "^4.1.1", + "react-is": "^16.8.1" + } + }, + "node_modules/prop-types/node_modules/react-is": { + "version": "16.13.1", + "resolved": "https://registry.npmjs.org/react-is/-/react-is-16.13.1.tgz", + "integrity": "sha512-24e6ynE2H+OKt4kqsOvNd8kBpV65zoxbA4BVsEOB3ARVWQki/DHzaUoC5KuON/BiccDaCCTZBuOcfZs70kR8bQ==", + "dev": true + }, "node_modules/propose": { "version": "0.0.5", "resolved": "https://registry.npmjs.org/propose/-/propose-0.0.5.tgz", @@ -35322,6 +35340,25 @@ "sisteransi": "^1.0.5" } }, + "prop-types": { + "version": "15.7.2", + "resolved": "https://registry.npmjs.org/prop-types/-/prop-types-15.7.2.tgz", + "integrity": "sha512-8QQikdH7//R2vurIJSutZ1smHYTcLpRWEOlHnzcWHmBYrOGUysKwSsrC89BCiFj3CbrfJ/nXFdJepOVrY1GCHQ==", + "dev": true, + "requires": { + "loose-envify": "^1.4.0", + "object-assign": "^4.1.1", + "react-is": "^16.8.1" + }, + "dependencies": { + "react-is": { + "version": "16.13.1", + "resolved": "https://registry.npmjs.org/react-is/-/react-is-16.13.1.tgz", + "integrity": "sha512-24e6ynE2H+OKt4kqsOvNd8kBpV65zoxbA4BVsEOB3ARVWQki/DHzaUoC5KuON/BiccDaCCTZBuOcfZs70kR8bQ==", + "dev": true + } + } + }, "propose": { "version": "0.0.5", "resolved": "https://registry.npmjs.org/propose/-/propose-0.0.5.tgz", diff --git a/package.json b/package.json index e5286929..dee2be46 100644 --- a/package.json +++ b/package.json @@ -2,7 +2,7 @@ "name": "pleasantest", "version": "0.6.1", "engines": { - "node": "12 || 14 || 16" + "node": "^12.2 || 14 || 16" }, "files": [ "dist" @@ -38,6 +38,7 @@ "polka": "0.5.2", "preact": "10.5.14", "prettier": "2.3.2", + "prop-types": "^15.7.2", "react": "17.0.2", "react-dom": "17.0.2", "remark-cli": "9.0.0", diff --git a/src/module-server/plugins/npm-plugin.ts b/src/module-server/plugins/npm-plugin.ts index 4ede65e0..2b195253 100644 --- a/src/module-server/plugins/npm-plugin.ts +++ b/src/module-server/plugins/npm-plugin.ts @@ -1,14 +1,14 @@ import { dirname, join, normalize, posix } from 'path'; import type { Plugin, RollupCache } from 'rollup'; import { rollup } from 'rollup'; -import { existsSync, promises as fs } from 'fs'; +import { promises as fs } from 'fs'; import { resolve, legacy as resolveLegacy } from 'resolve.exports'; import commonjs from '@rollup/plugin-commonjs'; import { processGlobalPlugin } from './process-global-plugin'; import * as esbuild from 'esbuild'; import { parse } from 'cjs-module-lexer'; -import MagicString from 'magic-string'; import { fileURLToPath } from 'url'; +import { createRequire } from 'module'; import { jsExts } from '../middleware/js'; import { changeErrorMessage } from '../../utils'; @@ -78,9 +78,9 @@ export const npmPlugin = ({ root }: { root: string }): Plugin => { const cachePath = join(cacheDir, '@npm', `${resolved.idWithVersion}.js`); const cached = await getFromCache(cachePath); if (cached) return cached; - const result = await bundleNpmModule(resolved.path, false); + const result = await bundleNpmModule(resolved.path, id, false); // Queue up a second-pass optimized/minified build - bundleNpmModule(resolved.path, true).then((optimizedResult) => { + bundleNpmModule(resolved.path, id, true).then((optimizedResult) => { setInCache(cachePath, optimizedResult); }); setInCache(cachePath, result); @@ -89,17 +89,16 @@ export const npmPlugin = ({ root }: { root: string }): Plugin => { }; }; -const nodeResolve = async (id: string, root: string) => { - const pathChunks = id.split(posix.sep); - const isNpmNamespace = id[0] === '@'; - const packageName = pathChunks.slice(0, isNpmNamespace ? 2 : 1); - // If it is an npm namespace, then get the first two folders, otherwise just one - const pkgDir = join(root, 'node_modules', ...packageName); - await fs.stat(pkgDir).catch(() => { - throw new Error(`Could not resolve ${id} from ${root}`); - }); - // Path within imported module - const subPath = join(...pathChunks.slice(isNpmNamespace ? 2 : 1)); +interface ResolveResult { + path: string; + idWithVersion: string; +} + +const resolveFromFolder = async ( + pkgDir: string, + subPath: string, + packageName: string[], +): Promise => { const pkgJsonPath = join(pkgDir, 'package.json'); let pkgJson; try { @@ -133,31 +132,63 @@ const nodeResolve = async (id: string, root: string) => { if (!result && subPath === '.') result = resolveLegacy(pkgJson, { browser: false, fields: ['main'] }); - if (!result) { + if (!result && !('exports' in pkgJson)) { const extensions = ['.js', '/index.js', '.cjs', '/index.cjs']; + // If this was not conditionally included, this would have infinite recursion + if (subPath !== '.') extensions.unshift(''); for (const extension of extensions) { const path = normalize(join(pkgDir, subPath) + extension); - if (existsSync(path)) return { path, idWithVersion }; + const stats = await fs.stat(path).catch(() => null); + if (stats) { + if (stats.isFile()) return { path, idWithVersion }; + if (stats.isDirectory()) { + // If you import some-package/foo and foo is a folder with a package.json in it, + // resolve main fields from the package.json + const result = await resolveFromFolder(path, '.', packageName); + if (result) return { path: result.path, idWithVersion }; + } + } } - - throw new Error(`Could not resolve ${id}`); } + if (!result) return false; return { path: join(pkgDir, result), idWithVersion }; }; +const resolveCache = new Map(); + +const resolveCacheKey = (id: string, root: string) => `${id}\0\0${root}`; + +const nodeResolve = async (id: string, root: string) => { + const cacheKey = resolveCacheKey(id, root); + const cached = resolveCache.get(cacheKey); + if (cached) return cached; + const pathChunks = id.split(posix.sep); + const isNpmNamespace = id[0] === '@'; + const packageName = pathChunks.slice(0, isNpmNamespace ? 2 : 1); + // If it is an npm namespace, then get the first two folders, otherwise just one + const pkgDir = join(root, 'node_modules', ...packageName); + await fs.stat(pkgDir).catch(() => { + throw new Error(`Could not resolve ${id} from ${root}`); + }); + // Path within imported module + const subPath = join(...pathChunks.slice(isNpmNamespace ? 2 : 1)); + const result = await resolveFromFolder(pkgDir, subPath, packageName); + if (result) { + resolveCache.set(cacheKey, result); + return result; + } + + throw new Error(`Could not resolve ${id}`); +}; + const pluginNodeResolve = (): Plugin => { return { name: 'node-resolve', resolveId(id) { if (isBareImport(id)) return { id: prefix + id, external: true }; - if (id.startsWith(prefix)) { - return { - // Remove the leading slash, otherwise rollup turns it into a relative path up to disk root - id, - external: true, - }; - } + // If requests already have the npm prefix, mark them as external + if (id.startsWith(prefix)) return { id, external: true }; }, }; }; @@ -166,58 +197,60 @@ let npmCache: RollupCache | undefined; /** * Bundle am npm module entry path into a single file - * @param mod The module to bundle, including subpackage/path + * @param mod The full path of the module to bundle, including subpackage/path + * @param id The imported identifier * @param optimize Whether the bundle should be a minified/optimized bundle, or the default quick non-optimized bundle */ -const bundleNpmModule = async (mod: string, optimize: boolean) => { +const bundleNpmModule = async (mod: string, id: string, optimize: boolean) => { + let namedExports: string[] = []; + if (dynamicCJSModules.has(id)) { + let isValidCJS = true; + try { + const text = await fs.readFile(mod, 'utf8'); + // Goal: Determine if it is ESM or CJS. + // Try to parse it with cjs-module-lexer, if it fails, assume it is ESM + // eslint-disable-next-line @cloudfour/typescript-eslint/await-thenable + await parse(text); + } catch { + isValidCJS = false; + } + + if (isValidCJS) { + const require = createRequire(import.meta.url); + // eslint-disable-next-line @cloudfour/typescript-eslint/no-var-requires + const imported = require(mod); + if (typeof imported === 'object' && !imported.__esModule) + namedExports = Object.keys(imported); + } + } + + const virtualEntry = '\0virtualEntry'; + const hasSyntheticNamedExports = namedExports.length > 0; const bundle = await rollup({ - input: mod, + input: hasSyntheticNamedExports ? virtualEntry : mod, cache: npmCache, shimMissingExports: true, treeshake: true, preserveEntrySignatures: 'allow-extension', plugins: [ - { - // This plugin fixes cases of module.exports = require('...') - // By default, the named exports from the required module are not generated - // This plugin detects those exports, - // and makes it so that @rollup/plugin-commonjs can see them and turn them into ES exports (via syntheticNamedExports) - // This edge case happens in React, so it was necessary to fix it. - name: 'cjs-module-lexer', - async transform(code, id) { - if (id.startsWith('\0')) return; - const out = new MagicString(code); - const re = - /(^|[\s;])module\.exports\s*=\s*require\(["']([^"']*)["']\)($|[\s;])/g; - let match; - while ((match = re.exec(code))) { - const [, leadingWhitespace, moduleName, trailingWhitespace] = match; - - const resolved = await this.resolve(moduleName, id); - if (!resolved || resolved.external) return; - - try { - const text = await fs.readFile(resolved.id, 'utf8'); - // eslint-disable-next-line @cloudfour/typescript-eslint/await-thenable - const parsed = await parse(text); - let replacement = ''; - for (const exportName of parsed.exports) { - replacement += `\nmodule.exports.${exportName} = require("${moduleName}").${exportName}`; - } - - out.overwrite( - match.index, - re.lastIndex, - leadingWhitespace + replacement + trailingWhitespace, - ); - } catch { - return; + hasSyntheticNamedExports && + ({ + // This plugin handles special-case packages whose named exports cannot be found via static analysis + // For these packages, the package is require()'d, and the named exports are determined that way. + // A virtual entry exports the named exports from the real entry package + name: 'cjs-named-exports', + resolveId(id) { + if (id === virtualEntry) return virtualEntry; + }, + load(id) { + if (id === virtualEntry) { + const code = `export * from '${mod}' +export {${namedExports.join(', ')}} from '${mod}' +export { default } from '${mod}'`; + return code; } - } - - return out.toString(); - }, - } as Plugin, + }, + } as Plugin), pluginNodeResolve(), processGlobalPlugin({ NODE_ENV: 'development' }), commonjs({ @@ -247,3 +280,9 @@ const bundleNpmModule = async (mod: string, optimize: boolean) => { return output[0].code; }; + +/** + * Any package names in this set will need to have their named exports detected manually via require() + * because the export names cannot be statically analyzed + */ +const dynamicCJSModules = new Set(['prop-types', 'react-dom', 'react']); diff --git a/tests/utils/runJS.test.tsx b/tests/utils/runJS.test.tsx index 6993a5b3..5baa2842 100644 --- a/tests/utils/runJS.test.tsx +++ b/tests/utils/runJS.test.tsx @@ -209,27 +209,6 @@ test.todo( ); test.todo('resolution error if a package does not exist in node_modules'); -test( - 'react and react-dom can be imported, and JSX works', - withBrowser(async ({ utils, screen }) => { - await utils.runJS(` - import * as React from 'react' - import React2 from 'react' - if (React.createElement !== React2.createElement) { - throw new Error('Namespace import did not yield same result as direct import') - } - import { render } from 'react-dom' - - const root = document.createElement('div') - document.body.innerHTML = '' - document.body.append(root) - render(

Hi

, root) - `); - const heading = await screen.getByRole('heading'); - await expect(heading).toHaveTextContent('Hi'); - }), -); - test( 'Allows importing CSS into JS file', withBrowser(async ({ utils, screen }) => { @@ -242,3 +221,43 @@ test( await expect(heading).not.toBeVisible(); }), ); + +describe('CJS interop edge cases', () => { + test( + 'Named exports implicitly created from default-only export', + withBrowser(async ({ utils }) => { + // Prop-types is CJS and provides non-statically-analyzable named exports + await utils.runJS(` + import { number } from 'prop-types' + import PropTypes from 'prop-types' + if (number !== PropTypes.number) { + throw new Error('Named import did not yield same result as default import') + } + PropTypes.checkPropTypes( + { name: number }, + { name: 5 }, + ); + `); + }), + ); + test( + 'react and react-dom can be imported, and JSX works', + withBrowser(async ({ utils, screen }) => { + await utils.runJS(` + import * as React from 'react' + import React2 from 'react' + if (React.createElement !== React2.createElement) { + throw new Error('Namespace import did not yield same result as direct import') + } + import { render } from 'react-dom' + + const root = document.createElement('div') + document.body.innerHTML = '' + document.body.append(root) + render(

Hi

, root) + `); + const heading = await screen.getByRole('heading'); + await expect(heading).toHaveTextContent('Hi'); + }), + ); +});