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.

Sign up for GitHub

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

Jump to bottom

Fix crashing with Babel polyfill / Chrome <= v37.0 #292

Merged
merged 2 commits into from
May 17, 2020
Merged

Fix crashing with Babel polyfill / Chrome <= v37.0 #292

merged 2 commits into from
May 17, 2020

Conversation

raido
Copy link
Contributor

@raido raido commented Jan 18, 2020 •
edited
Loading

Closes: #291

It seems with Babel polyfill included Object.isFrozen vs Object.isExtensible has different meaning:

chrome-37-original-failure

This change was tested on:

  • IE 10
  • Chrome v27+
  • Chrome latest
  • IE 9 (has some other non related failure)

See screenshots below:

IE 9:
ie9

IE 10:
ie10

Chrome v27
chrome-27

@raido raido mentioned this pull request Jan 18, 2020
Closed
@raido
Copy link
Contributor Author

raido commented Jan 18, 2020

Travis failed because transitive package seems to have dropped Node 6 support.

rwjblue
rwjblue reviewed Jan 25, 2020
View reviewed changes
lib/router/utils.ts Outdated
* @private
*/
export function isFrozenOrNotExtensible(obj: object): boolean {
return Object.isFrozen(obj) || !Object.isExtensible(obj);
Copy link
Collaborator

Choose a reason for hiding this comment

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

When is !Object.isExtensible(obj) not enough? Specifically, I'm wondering if !Object.extensible(obj) can ever be true for frozen objects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. I changed it to just !Object.isExtensible(obj) and ran on same browsers (with Babel polyfill included) and tests pass.

I'll try to see if I can get Travis happy as well if not then I guess have to merge it as is?

@raido
Copy link
Contributor Author

raido commented Jan 25, 2020 •
edited
Loading

I switched Travis from Node 6 -> 8.

We should drop Node <= 6 and release v7 of this package, after releasing v6.2.6 with this PR and backporting all the way to Ember.js v3.12 LTS over here - https://github.com/emberjs/ember.js/blob/master/package.json#L140

Since Ember.js side uses caret for router_js version ,then simply yarn upgrade router_js would be enough but I am happy to open PR and explicitly set version to v6.2.6.

And then for Ember canary include v7 of this package.

@raido raido requested a review from rwjblue January 25, 2020 20:11
@raido
Copy link
Contributor Author

raido commented Feb 6, 2020

Any updates on this?

@raido
Copy link
Contributor Author

raido commented May 17, 2020

@rwjblue ping :)

@rwjblue rwjblue merged commit d8231d3 into tildeio:master May 17, 2020
@rwjblue
Copy link
Collaborator

rwjblue commented May 17, 2020

Thank you @raido!

@rwjblue rwjblue added the bug label Jul 20, 2020
@richgt richgt mentioned this pull request Jul 20, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwjblue rwjblue rwjblue approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Can't add property attributes, object is not extensible
2 participants
@raido @rwjblue