Skip to content

Fix detecting an existing Map/Set #47409

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

Merged
merged 1 commit into from
Jan 24, 2022

Conversation

elibarzilay
Copy link
Contributor

This didn't affect compilation to CJS since that sets exports.Map
instead of creating a global.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jan 12, 2022
@rbuckton
Copy link
Contributor

The point of the declare const Map on the preceding line is to force the lookup of Map to look at the globally scoped Map (rather than ts.Map due to namespace merging). How did this become an error for CJS emit?

@elibarzilay
Copy link
Contributor Author

The problem is not in the CJS output. If I set module to es6, the resulting code is:

var NativeCollections;
(function (NativeCollections) {
    function tryGetNativeMap() {
        return typeof Map !== "undefined" && ...;
    }
    ...
})(NativeCollections || (NativeCollections = {}));
export var Map = getCollectionImplementation("Map", "tryGetNativeMap", "createMapShim");
```

@elibarzilay
Copy link
Contributor Author

@rbuckton, does that make sense?

@elibarzilay elibarzilay force-pushed the fix-map-set-shims branch 2 times, most recently from c1beaa1 to bb2334f Compare January 24, 2022 19:36
Copy link
Contributor

@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.

There's a few more small issues I noticed, after which this should be ready to merge.

Comment on lines 119 to 122
const globals = typeof globalThis !== "undefined" ? globalThis
: typeof global !== "undefined" ? global
: typeof self !== "undefined" ? self
: undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor formatting nit:

Suggested change
const globals = typeof globalThis !== "undefined" ? globalThis
: typeof global !== "undefined" ? global
: typeof self !== "undefined" ? self
: undefined;
const globals = typeof globalThis !== "undefined" ? globalThis :
typeof global !== "undefined" ? global :
typeof self !== "undefined" ? self :
undefined;


/**
* Returns the native Map implementation if it is available and compatible (i.e. supports iteration).
*/
export function tryGetNativeMap(): MapConstructor | undefined {
// Internet Explorer's Map doesn't support iteration, so don't use it.
const gMap = globals.Map;
Copy link
Contributor

Choose a reason for hiding this comment

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

globals is possibly undefined.

Suggested change
const gMap = globals.Map;
const gMap = globals?.Map;

}

/**
* Returns the native Set implementation if it is available and compatible (i.e. supports iteration).
*/
export function tryGetNativeSet(): SetConstructor | undefined {
// Internet Explorer's Set doesn't support iteration, so don't use it.
const gSet = globals.Set;
Copy link
Contributor

Choose a reason for hiding this comment

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

globals is possibly undefined.

Suggested change
const gSet = globals.Set;
const gSet = globals?.Set;

This didn't affect compilation to CJS since that sets `exports.Map`
instead of creating a global.
@elibarzilay elibarzilay merged commit 6927730 into microsoft:main Jan 24, 2022
@elibarzilay elibarzilay deleted the fix-map-set-shims branch January 24, 2022 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants