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

[BUGFIX canary]: Remove the last Symbols #6428

Merged
merged 5 commits into from
Sep 13, 2019
Merged

[BUGFIX canary]: Remove the last Symbols #6428

merged 5 commits into from
Sep 13, 2019

Conversation

dcyriller
Copy link
Contributor

Description

This is an attempt at removing the last Symbols from master. The goal is to fix IE compatibility regarding Symbol support, as detailed in this issue: #6380.

It won't have to be backported. Indeed, the Identifier feature has been turned on in #6366 and included in a tag for the first time in v3.14.0-alpha.2.

Thank you for pointing me to that solution @rwjblue (#6389).

Concerns

With the implementation I've done, I'm concerned by the fact:

  • it kind of changes the StableIdentifier interface
  • the isStableIdentifier signature is modified

But as it is still on canary, it might be fine?

Is the symbol fix good enough? It has been suggested it could be replaced by the WeakMap fix every time symbol is used.

Tests

I've run the tests with the command: yarn test. Identifier feature flag being turned on, the test seemed to be fine. I'd be happy to add more if needed.

@dcyriller
Copy link
Contributor Author

External Partner Travis is failing in Azure, but not in Travis.

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.

The existing work is good however we will want to ensure that the identifier is removed from the WeakMap when removed from the identifiers cache here https://github.com/emberjs/data/blob/master/packages/store/addon/-private/identifiers/cache.ts#L376

(We should do this because although WeakMap's allow for key GC they don't guarantee it)

@rwjblue
Copy link
Member

rwjblue commented Sep 11, 2019

Ya, if there are for sure places where we know we can remove, we totally should.

identifier from the WeakMap when it is removed from the identifier
cache.
@dcyriller
Copy link
Contributor Author

we will want to ensure that the identifier is removed from the WeakMap when removed from the identifiers cache

It's added.

Also, I've added a naive unit test for is-stable-identifier module. But I've failed on the module's import statement:

Could not find module `@ember-data/store/addon/-private/identifiers/is-stable-identifier` imported from `dummy/tests/unit/identifiers/utils/is-stable-identifier-test`

What am I missing?

@dcyriller
Copy link
Contributor Author

Whoo tests are passing. What do you think of the added test @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.

Overall looks good, I'd like to use getOrCreateRecordIdentifier as we're testing a resource-hash with a constant ID, and add one assertion.

@runspired runspired merged commit 88dcf5f into emberjs:master Sep 13, 2019
@dcyriller dcyriller deleted the replace-identifier-symbol branch September 13, 2019 18:59
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.

3 participants