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.

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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ExE-Boss
Copy link
Contributor

Babel has been doing this since babel/babel#10161, babel/babel#10574 and babel/babel#10585.

tslib.es6.js Outdated Show resolved Hide resolved
Copy link
Member

@rbuckton rbuckton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could also be implemented such that you don't need to repeatedly call __getImportStarCache using something like this:

__importStar = function (mod) {
  var cache = typeof WeakMap === "function" ? new WeakMap() : null;
  __importStar = function (mod) {
    ...
  };
  return _importStar(mod);
};

Which is similar to what we do for extendsStatics.

tslib.es6.js Outdated Show resolved Hide resolved
tslib.js Outdated Show resolved Hide resolved
@ExE-Boss ExE-Boss force-pushed the feat/import-star/cache-results branch from 19622b4 to 27c4b32 Compare January 6, 2021 12:14
@ExE-Boss ExE-Boss requested a review from rbuckton January 6, 2021 12:15
return result;
}
export var __importStar = function (mod) {
var cache = typeof WeakMap === "function" ? new WeakMap() : null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you proposing this be implemented in TypeScript as well for inline helper emit? If so we'd need to introduce a checker error if you declare your own value named WeakMap at the top of a module.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue there is that this is supposed to be an optimisation when two different modules that use tslib import the same CommonJS module, the value returned by __importStar is the same value for both imports, e.g.:

// modA.js
Object.defineProperty(exports, "__esModule", { value: true });
var tslib = require("tslib");

exports.example = tslib.__importStar(require("example"));
// modB.js
Object.defineProperty(exports, "__esModule", { value: true });
var tslib = require("tslib");

exports.example = tslib.__importStar(require("example"));
// test.js
var assert = require("assert");

var modA = require("./modA.js");
var modB = require("./modA.js");

// The following passes regardless of whether `example` has `__esModule` or not:
assert.strictEqual(modA.example, modB.example);

It doesn’t make sense to include the module cache in inline helpers, as modA and modB would then each have their own cache, and every new local __imporStar call would then have a cache miss, since the cache wouldn’t be global.

export var __importStar = function (mod) {
var cache = typeof WeakMap === "function" ? new WeakMap() : null;
__importStar = function (mod) {
if (mod === null || (typeof mod !== "object" && typeof mod !== "function")) return { default: mod };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Parens aren't needed here, since && has higher precedence than ||.

Suggested change
if (mod === null || (typeof mod !== "object" && typeof mod !== "function")) return { default: mod };
if (mod === null || typeof mod !== "object" && typeof mod !== "function") return { default: mod };

return result;
var cache = typeof WeakMap === "function" ? new WeakMap() : null;
__importStar = function (mod) {
if (mod === null || (typeof mod !== "object" && typeof mod !== "function")) return { default: mod };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Parens aren't necessary here.

Suggested change
if (mod === null || (typeof mod !== "object" && typeof mod !== "function")) return { default: mod };
if (mod === null || typeof mod !== "object" && typeof mod !== "function") return { default: mod };

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants