fix: _interopRequireWildcard should only cache objects by samMeow · Pull Request #10574 · babel/babel

samMeow · 2019-10-18T13:14:51Z

Q	A
Fixed Issues?	Fixes #10561
Patch: Bug Fix?	Yes
Major: Breaking Change?	No
Minor: New Feature?	No
Tests Added + Pass?	Yes
Documentation PR Link	No
Any Dependency Changes?	No
License	MIT

_interopRequireWildcard (used for dynamic import) wrongly cached string modules (e.g. base64 image) to Weakmap throws error. This fix is to cache object only for the function.

buildsize · 2019-10-18T13:26:02Z

File name	Previous Size	New Size	Change
babel-preset-env.js	2.73 MB	2.73 MB	136 bytes (0%)
babel-preset-env.min.js	1.65 MB	1.65 MB	132 bytes (0%)
babel.js	2.92 MB	2.92 MB	136 bytes (0%)
babel.min.js	1.61 MB	1.61 MB	132 bytes (0%)
test262.tap	4.84 MB	[deleted]

nicolo-ribaudo · 2019-10-18T14:10:19Z

packages/babel-helpers/src/helpers.js

      return obj;
    }

+    if (!(obj instanceof Object)) {


This isn't safe, for example, if __proto__ is null. You should use typeof.

I wish we can use _.isObject in the helpers. I copied from there

typeof obj === "object" || typeof obj === "function"

because (new WeakMap).set(new Function, 42) is valid.

Thanks for the advices. Inspiring from _.isObject. We should also add null check, because typeof null === 'object' and (new WeakMap).set(null, { default: null }) throws error.

Nicolò points out a potential issue.

theKashey · 2019-10-19T06:03:27Z

packages/babel-helpers/src/helpers.js

      return obj;
    }

+    if (obj === null || (typeof obj !== "object" && typeof obj !== "function")) {


What about adding tests for this? It would be easier to understand how this function should work, and prevent possible issues in the future.

For test case, you may refer to packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/*. You will find an example for function module exec-interop and newly added exec-interop-string, exec-interop-null.

theKashey · 2019-10-19T06:28:04Z

Sorry for the offtopic, but are you sure it's still working correctly?

For example, lets double check construction like import * as classnames from 'classnames'
classnames are defined in a more or less common way, for many "old" packages

classNames.default = classNames;
module.exports = classNames;

So let's imagine - import * as classnames from 'classnames' (* required by TS, as long as there is no(properly defined) "default" import)

Q: What webpack would emit:
A: var classnames__WEBPACK_IMPORTED_MODULE_0__ = __webpack_require__(/*! classnames */ "./node_modules/classnames/index.js");
Q: What is it?
A: It's a function you can use
Q: What would happen next?
A Nothing bad

Q: What parcel would emit (uses babel):
A: var classnames = _interopRequireWildcard(require("classnames"));
Q: What is it?
A: It's a {default: classNames}
Q: What would happen next?
A: classnames is not a function.

Q: What would happen if import classnames from 'classnames'?
A: It would work, but that's not correct.

So, question - what _interopRequireWildcard is doing? Why it's trying to return an object with .default

As far as I understand - the inner loop creates an object clone, plus stores the original object as default. Look like that's working for objects, but not for the functions.

So:

_interopRequireWildcard(1) == 1
_interopRequireWildcard(false) == false
_interopRequireWildcard("hey") == "hey
_interopRequireWildcard({test: 42}).test == 42
_interopRequireWildcard(function(){return 'test')) === ???

Right now compiling the same file using webpack or parcel(babel) produces not only different code - it's not working in the second case.

nicolo-ribaudo · 2019-10-19T06:39:10Z

import * as x must always return an object. If webpack returns a function, it's a webpack bug. (To be 100% correct, it should be an object with __proto__ === null and read-only)

In your example, import classnames from 'classnames' is correct.

theKashey · 2019-10-19T06:57:31Z

Is it a part of ESM specification? As long as that would be TypeScript bug in this case, and all the projects using TypeScript and wildcard imports to read exports as is from commonjs module, and exposing ESM modules as the output... are broken.
Well, this is how I got into this issue - ‘import * as isNode’ from “detect-node”’ - and no default export - https://unpkg.com/browse/@types/detect-node@2.0.0/index.d.ts

nicolo-ribaudo · 2019-10-19T09:41:00Z

@theKashey Yes, the specification defines imported namespaces (with * as foo) as that kind of exotic objects. The default exported by a module is accessible as foo.default.
That object is defined at https://tc39.es/ecma262/#table-29. As you can see at https://tc39.es/ecma262/#function-object, a function is an object with an internal [[Call]] property, which imported namespaces don't have.

Also note that Babel aligns with Node's implementation in this case: the default export of a module is it's module.exports.

ExE-Boss · 2019-10-22T06:41:02Z

There’s a second non‑nullish check a few lines after this, so I’m removing it in #10585, since it’s no longer necessary.

JLHwung previously approved these changes Oct 18, 2019

View reviewed changes

existentialism approved these changes Oct 18, 2019

View reviewed changes

nicolo-ribaudo requested changes Oct 18, 2019

View reviewed changes

jridgewell approved these changes Oct 19, 2019

View reviewed changes

jridgewell mentioned this pull request Oct 19, 2019

_interopRequireWildcard breaks on nodejs cases #10577

Closed

samMeow added 2 commits October 19, 2019 13:33

fix: [#10561] _interopRequireWildcard should cache object only

684fb60

fix: narrow down cache restriction to object/function type

0504e9f

theKashey reviewed Oct 19, 2019

View reviewed changes

theKashey mentioned this pull request Oct 19, 2019

Not compatible with TypeScript JedWatson/classnames#205

Closed

test: add interop require null module case

118760e

nicolo-ribaudo approved these changes Oct 19, 2019

View reviewed changes

nicolo-ribaudo changed the title ~~fix: [#10561] _interopRequireWildcard should cache object only~~ fix: _interopRequireWildcard should only cache objects Oct 19, 2019

nicolo-ribaudo merged commit fe258de into babel:master Oct 19, 2019

nicolo-ribaudo added i: regression PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Oct 19, 2019

ExE-Boss mentioned this pull request Oct 22, 2019

fix(babel‑helpers/interopRequireWildcard): Avoid double nullish check #10585

Merged

ExE-Boss mentioned this pull request Oct 24, 2019

feat(importStar): Cache non‑module results microsoft/tslib#80

Open

snyk-bot mentioned this pull request Nov 14, 2019

[Snyk] Upgrade @babel/runtime from 7.6.3 to 7.7.1 cds-snc/report-a-cybercrime#936

Merged

This was referenced Nov 28, 2019

[Snyk] Upgrade @babel/runtime from 7.0.0-beta.42 to 7.7.2 ali8889/mist#4

Open

[Snyk] Upgrade @babel/runtime from 7.5.5 to 7.7.4 dyna-dot/CyberChef#3

Open

This was referenced Dec 5, 2019

[Snyk] Upgrade @babel/runtime from 7.3.1 to 7.7.4 dyna-dot/bit-javascript#5

Open

[Snyk] Upgrade @babel/runtime from 7.5.5 to 7.7.4 jakeherp/todo-list#1

Merged

snyk-bot mentioned this pull request Jan 12, 2020

[Snyk] Upgrade @babel/runtime from 7.4.4 to 7.7.7 mboocannon/msgraph-sdk-javascript#1

Open

lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jan 24, 2020

lock bot locked as resolved and limited conversation to collaborators Jan 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: _interopRequireWildcard should only cache objects#10574

fix: _interopRequireWildcard should only cache objects#10574
nicolo-ribaudo merged 3 commits intobabel:masterfrom
samMeow:fix/require_wildcard_string_10561

samMeow commented Oct 18, 2019

Uh oh!

buildsize bot commented Oct 18, 2019 •

edited

Loading

Uh oh!

nicolo-ribaudo Oct 18, 2019

Uh oh!

JLHwung Oct 18, 2019

Uh oh!

samMeow Oct 19, 2019

Uh oh!

theKashey Oct 19, 2019

Uh oh!

samMeow Oct 19, 2019 •

edited

Loading

Uh oh!

theKashey commented Oct 19, 2019

Uh oh!

nicolo-ribaudo commented Oct 19, 2019 •

edited

Loading

Uh oh!

theKashey commented Oct 19, 2019

Uh oh!

nicolo-ribaudo commented Oct 19, 2019

Uh oh!

ExE-Boss commented Oct 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Uh oh!

Conversation

samMeow commented Oct 18, 2019

Uh oh!

buildsize bot commented Oct 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolo-ribaudo Oct 18, 2019

Choose a reason for hiding this comment

Uh oh!

JLHwung Oct 18, 2019

Choose a reason for hiding this comment

Uh oh!

samMeow Oct 19, 2019

Choose a reason for hiding this comment

Uh oh!

theKashey Oct 19, 2019

Choose a reason for hiding this comment

Uh oh!

samMeow Oct 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theKashey commented Oct 19, 2019

Uh oh!

nicolo-ribaudo commented Oct 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theKashey commented Oct 19, 2019

Uh oh!

nicolo-ribaudo commented Oct 19, 2019

Uh oh!

ExE-Boss commented Oct 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

buildsize bot commented Oct 18, 2019 •

edited

Loading

samMeow Oct 19, 2019 •

edited

Loading

nicolo-ribaudo commented Oct 19, 2019 •

edited

Loading