From 71a5b351a7b6601ac1cda20130060ed1ef56b743 Mon Sep 17 00:00:00 2001 From: yepitschunked <125177+yepitschunked@users.noreply.github.com> Date: Tue, 17 Jan 2023 04:37:07 -0800 Subject: [PATCH] fix(shaker): workaround for weirdly packaged cjs modules (#1178) * fix: workaround for weirdly packaged cjs modules * chore: changeset for #1178 Co-authored-by: Victor Lin Co-authored-by: Anton Evzhakov --- .changeset/shiny-owls-visit.md | 7 ++ .../plugins/__tests__/shaker-plugin.test.ts | 98 +++++++++++++++++++ packages/shaker/src/plugins/shaker-plugin.ts | 15 ++- packages/testkit/src/module.test.ts | 1 + .../utils/src/collectExportsAndImports.ts | 34 ++++--- packages/vite/src/index.ts | 2 +- 6 files changed, 142 insertions(+), 15 deletions(-) create mode 100644 .changeset/shiny-owls-visit.md diff --git a/.changeset/shiny-owls-visit.md b/.changeset/shiny-owls-visit.md new file mode 100644 index 000000000..03dce9441 --- /dev/null +++ b/.changeset/shiny-owls-visit.md @@ -0,0 +1,7 @@ +--- +'@linaria/shaker': patch +'@linaria/testkit': patch +'@linaria/utils': patch +--- + +Workaround for weirdly packaged cjs modules. diff --git a/packages/shaker/src/plugins/__tests__/shaker-plugin.test.ts b/packages/shaker/src/plugins/__tests__/shaker-plugin.test.ts index 8096cf936..4fab5333b 100644 --- a/packages/shaker/src/plugins/__tests__/shaker-plugin.test.ts +++ b/packages/shaker/src/plugins/__tests__/shaker-plugin.test.ts @@ -196,6 +196,7 @@ describe('shaker', () => { expect(code).toMatchSnapshot(); expect(metadata.imports.size).toBe(0); }); + it('should handle object patterns in exports', () => { const { code } = keep(['Alive'])` import foo from "foo"; @@ -211,4 +212,101 @@ describe('shaker', () => { } = foo();" `); }); + + it('avoids deleting non-default exports when importing default export of a module without an __esModule: true property', () => { + /* without workaround, this will be transformed by shaker to: + const n = require('n'); + const defaultExports = { + createContext: n.createContext + }; + exports.default = defaultExports; + + i.e, exports.createContext is deleted + */ + const { code } = keep(['default'])` + const n = require('n'); + const defaultExports = { createContext: n.createContext } + Object.defineProperty(exports, "createContext", { + enumerable: !0, + get: function() { + return n.createContext + } + }) + exports.default = defaultExports; + `; + + /* + this breaks babel's interopRequireDefault + without shaker, interopRequireDefault() + returns: { + default: { + default: { createContext: ... } + createContext: ... + } + } + The double-default is because the module does not define + __esModule: true, so interopRequireDefault assumes that it needs to wrap the exports in { default: ... }, so that subsequent code can access createContext via `.default.createContext`. + + If shaker treats the createExport named export as unused (since it's not in onlyExports), it will delete it. interopRequireDefault() + returns: { + default: { + default: { createContext: ... } + } + } + + And `.default.createContext` will be undefined. + + Therefore, we assert that createContext is not deleted in this case. + */ + expect(code).toMatchInlineSnapshot(` + "const n = require('n'); + const defaultExports = { + createContext: n.createContext + }; + Object.defineProperty(exports, \\"createContext\\", { + enumerable: !0, + get: function () { + return n.createContext; + } + }); + exports.default = defaultExports;" + `); + }); + + it('deletes non-default exports when importing default export of a module with an __esModule: true property', () => { + /* without workaround, this will be transformed by shaker to: + const n = require('n'); + const defaultExports = { + createContext: n.createContext + }; + exports.default = defaultExports; + + i.e, exports.createContext is deleted + */ + const { code } = keep(['default'])` + const n = require('n'); + const defaultExports = { createContext: n.createContext } + Object.defineProperty(exports, "__esModule", { + value: true + }); + Object.defineProperty(exports, "createContext", { + enumerable: !0, + get: function() { + return n.createContext + } + }) + exports.default = defaultExports; + `; + + expect(code).toMatchInlineSnapshot(` + "const n = require('n'); + const defaultExports = { + createContext: n.createContext + }; + Object.defineProperty(exports, \\"__esModule\\", { + value: true + }); + exports.default = defaultExports;" + `); + }); }); diff --git a/packages/shaker/src/plugins/shaker-plugin.ts b/packages/shaker/src/plugins/shaker-plugin.ts index 4353f0d77..170c97b3a 100644 --- a/packages/shaker/src/plugins/shaker-plugin.ts +++ b/packages/shaker/src/plugins/shaker-plugin.ts @@ -197,7 +197,20 @@ export default function shakerPlugin( return; } - + // Hackaround for packages which include a 'default' export without specifying __esModule; such packages cannot be + // shaken as they will break interopRequireDefault babel helper + // See example in shaker-plugin.test.ts + // Real-world example was found in preact/compat npm package + if ( + onlyExports.includes('default') && + exports.find(({ exported }) => exported === 'default') && + !collected.isEsModule + ) { + this.imports = collected.imports; + this.exports = exports; + this.reexports = collected.reexports; + return; + } if (!onlyExports.includes('*')) { const aliveExports = new Set(); const importNames = collected.imports.map(({ imported }) => imported); diff --git a/packages/testkit/src/module.test.ts b/packages/testkit/src/module.test.ts index 772f831e6..5763aca04 100644 --- a/packages/testkit/src/module.test.ts +++ b/packages/testkit/src/module.test.ts @@ -1,4 +1,5 @@ import path from 'path'; + import dedent from 'dedent'; import { Module, TransformCacheCollection } from '@linaria/babel-preset'; diff --git a/packages/utils/src/collectExportsAndImports.ts b/packages/utils/src/collectExportsAndImports.ts index c44953b3d..6e4439d4b 100644 --- a/packages/utils/src/collectExportsAndImports.ts +++ b/packages/utils/src/collectExportsAndImports.ts @@ -65,6 +65,7 @@ export interface IState { exports: IExport[]; imports: (IImport | ISideEffectImport)[]; reexports: IReexport[]; + isEsModule: boolean; } export const sideEffectImport = ( @@ -606,6 +607,8 @@ function collectFromExports(path: NodePath, state: IState): void { const { name } = property.node; if (name === '__esModule') { + // eslint-disable-next-line no-param-reassign + state.isEsModule = true; return; } @@ -623,21 +626,25 @@ function collectFromExports(path: NodePath, state: IState): void { if ( obj?.isIdentifier(path.node) && prop?.isStringLiteral() && - prop.node.value !== '__esModule' && descriptor?.isObjectExpression() ) { - /** - * Object.defineProperty(exports, "token", { - * enumerable: true, - * get: function get() { - * return _unknownPackage.token; - * } - * }); - */ - const exported = prop.node.value; - const local = getGetterValueFromDescriptor(descriptor); - if (local) { - state.exports.push({ exported, local }); + if (prop.node.value === '__esModule') { + // eslint-disable-next-line no-param-reassign + state.isEsModule = true; + } else { + /** + * Object.defineProperty(exports, "token", { + * enumerable: true, + * get: function get() { + * return _unknownPackage.token; + * } + * }); + */ + const exported = prop.node.value; + const local = getGetterValueFromDescriptor(descriptor); + if (local) { + state.exports.push({ exported, local }); + } } } else if ( obj?.isIdentifier(path.node) && @@ -1052,6 +1059,7 @@ export default function collectExportsAndImports( exports: [], imports: [], reexports: [], + isEsModule: false, }; if (!force && cache.has(path)) { diff --git a/packages/vite/src/index.ts b/packages/vite/src/index.ts index a4ce36bf9..c24829c97 100644 --- a/packages/vite/src/index.ts +++ b/packages/vite/src/index.ts @@ -43,7 +43,7 @@ export default function linaria({ // const targets: { id: string; dependencies: string[] }[] = []; const cache = new TransformCacheCollection(); - const { codeCache, resolveCache, evalCache } = cache; + const { codeCache, evalCache } = cache; return { name: 'linaria', enforce: 'post',