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

Change deprecated usages of Ember.create and Ember.keys to Object.create and Object.keys #98

Closed
wants to merge 2 commits into from

Conversation

akshayrawat
Copy link

Fixes #97

Tasks:

  • 1. Rename Ember.keys to Object.keys.
  • 2. Rename Ember.create to Object.create.
  • 3. Add polyfills for Object.create and Object.keys.

@akshayrawat
Copy link
Author

cc @stefanpenner

Not entirely sure if the generated dist needs to be commited. Let me know.

@stefanpenner
Copy link
Contributor

unfortunately for the time being this library still needs to support IE8. (Ember 2.0 wont support IE8 hence the deprecations) So in the interim, please be sure to include the appropriate polyfils

@akshayrawat
Copy link
Author

I thought that the polyfils were expected to be present on the page when this gets loaded, rather than it providing it. An assertion for that is here: https://github.com/ember-cli/ember-resolver/blob/master/packages/ember-resolver/lib/core.js#L36. Should we do something similar for Object.keys?

@stefanpenner
Copy link
Contributor

In some future release maybe, but for the 1.x series of ember that would be a breaking change. That check is to ensure the right polyfil is use, if one is used at all.

@akshayrawat
Copy link
Author

Got it. Since I'm new to this project, I've added a task list to the description above.

1 & 2 are done I think. Can you give me pointers on 3. And 4 is a question.

On 3: Should we create a new file ember-resolver/lib/polyfills.js with polyfills for Object.keys and Object.create. Mainly copied from here. Or should it be brought it as a bower dependency?

Thanks for getting me started on this project.

@rwjblue
Copy link
Member

rwjblue commented Jun 23, 2015

@akshayrawat - Inlining here seems fine for now, we need to rework the build system to something more modern and when we do that we can swap to bower.

@akshayrawat
Copy link
Author

@rwjblue Alright then this PR is all set. Let me know if I need to do anything else to wrap this up. Thanks.

@rwjblue
Copy link
Member

rwjblue commented Jun 23, 2015

I'm not seeing the polyfils here, did you forget to add them to git?

@stefanpenner
Copy link
Contributor

Be sure that the polyfils don't smash the globals, this should remain an isolated change

@akshayrawat
Copy link
Author

Added the polyfills. They were just copied from the source mentioned in the comments. The formatting rules for both are different, hence JSHint complains. Not sure if we should fix the formatting, or retain the original untouched copy. Let me know.

@akshayrawat
Copy link
Author

Not sure why the tests failed.

@stefanpenner
Copy link
Contributor

superseded by: #100 thanks for the PR though.

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.

None yet

3 participants