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

fix: Strip Symbol from production builds #6389

Merged
merged 2 commits into from
Aug 28, 2019
Merged

fix: Strip Symbol from production builds #6389

merged 2 commits into from
Aug 28, 2019

Conversation

dcyriller
Copy link
Contributor

@dcyriller dcyriller commented Aug 28, 2019

To ensure compatibility with IE11.

This should close #6380.

This implementation is taken from glimmer-vm. It is enumerable (as opposed to Symbol which is non-enumerable). But making it non-enumerable would come with a cost. And it will only be required for identifiers-related Symbols.

These remaining Symbols are still unpublished or behind a feature flag (not sure but they are not part of the build outputs). They are not included in this PR. As they'll require a more sophisticated approach to be specifically non-enumerable. As we said, I'll leave it to you @runspired.

Copy link
Contributor

@runspired runspired left a comment

Choose a reason for hiding this comment

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

As mentioned in the description, this doesn't change the Symbol usage for IS_IDENTIFIER on identifiers. I believe for that particular usage we do not want an enumerable symbol as we want to ensure ease of use and ease of serializability when handing them out to users (these other locations don't have this same constraint).

For IS_IDENTIFIER I suspect a weak-map lookup is what we want as more than likely that will be less expensive than defining a non-enumerable property, although I'm not 100% sure on this. cc @krisselden and @rwjblue for their thoughts on that.

Copy link
Member

rwjblue commented Aug 28, 2019

For IS_IDENTIFIER I suspect a weak-map lookup is what we want as more than likely that will be less expensive than defining a non-enumerable property, although I'm not 100% sure on this. cc @krisselden and @rwjblue for their thoughts on that.

Ya, I agree. I'd go with a WeakMap (I'd probably also just always use the WeakMap and not have separate branches).

@runspired runspired merged commit 84d510a into emberjs:master Aug 28, 2019
@dcyriller dcyriller deleted the bugfix-symbol-ie11 branch August 28, 2019 20:46
@dcyriller
Copy link
Contributor Author

I’ll update symbol implementation with WeakMap very soon (incl. for identifiers related Symbols).

@dcyriller
Copy link
Contributor Author

I've digged a bit and couldn't figure out how the WeakMap thing would look like. Could you give me another clue?

@rwjblue
Copy link
Member

rwjblue commented Sep 6, 2019

@dcyriller in general, the idea would be that instead of:

export const IS_IDENTIFIER = Symbol('is-identifier');

let recordIdentifier = {
[IS_IDENTIFIER]: true as const,
lid,
id,
type,
};

export default function isStableIdentifier(identifier: any): identifier is StableRecordIdentifier {
return identifier[IS_IDENTIFIER] === true;
}

You would do something akin to:

const IDENTIFIERS = new WeakMap();

export function isStableIdentifier(identifier) {
  return IDENTIFIERS.has(identifier);
}

export function markStableIdentifier(identifier) {
  IDENTIFIERS.set(identifier);
}

runspired pushed a commit that referenced this pull request Sep 11, 2019
* Add a Symbol-like symbol (thank you glimmer-vm)

This is similar to what is implemented here:
https://github.com/glimmerjs/glimmer-vm/blob/72718d0d6ebb2614ec984f51435a44830d410e4c/packages/%40glimmer/reference/lib/validators.ts

* Replace `Symbol`s that may be iterable by `symbol`s

These occurences of Symbol might be replaced by this `symbol`, similar
but different in the sense it is iterable.
runspired added a commit that referenced this pull request Sep 11, 2019
* Add a Symbol-like symbol (thank you glimmer-vm)

This is similar to what is implemented here:
https://github.com/glimmerjs/glimmer-vm/blob/72718d0d6ebb2614ec984f51435a44830d410e4c/packages/%40glimmer/reference/lib/validators.ts

* Replace `Symbol`s that may be iterable by `symbol`s

These occurences of Symbol might be replaced by this `symbol`, similar
but different in the sense it is iterable.
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.

ember-data@3.12 does not work in IE11
3 participants