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

Restructure Ember.libraries to be more idiomatic. Fixes #9291 #9312

Merged
merged 1 commit into from
Nov 21, 2014

Conversation

jayphelps
Copy link
Contributor

Requested by @stefanpenner #9291

@jayphelps jayphelps force-pushed the idiomatic-libraries branch 3 times, most recently from 29b6f9f to 2758a22 Compare October 13, 2014 04:48
@jayphelps
Copy link
Contributor Author

@stefanpenner Looks like this might have been in vain. ember-inspector makes the assumption that Ember.libraries is array-like in tons of places. This PR as-is will break that. So....thoughts?

https://github.com/emberjs/ember-inspector/blob/e7cc751cd5d934c6ac55159c6a94e1c2f7e8be24/dist_common/in-page-script.js#L7
https://github.com/emberjs/ember-inspector/blob/bda7a51b00fb243a4a1a3102dcfd89c1a47bdfa7/ember_debug/general_debug.js#L25

etc.

@jayphelps jayphelps changed the title Restructure Ember.libraries to be more idiomatic. Fixes #9291 [WIP] Restructure Ember.libraries to be more idiomatic. Fixes #9291 Oct 13, 2014
@jayphelps jayphelps force-pushed the idiomatic-libraries branch from 2758a22 to 87368fe Compare October 14, 2014 21:15
@jayphelps
Copy link
Contributor Author

Passing now. Had to account for Ember.warn being stripped in prod builds.

@rwjblue
Copy link
Member

rwjblue commented Oct 14, 2014

@jayphelps - the concern about ember-inspector still needs to be addressed right?

@stefanpenner
Copy link
Member

cc @teddyzeenny

@jayphelps
Copy link
Contributor Author

@rwjblue Yep. Needs to be addressed either in this PR or inspector. Maybe a combination of both during a transitional phase.

One thought..I can mock the array-like behavior, adding deprecation warnings for anyone else who might happen to be depending on that. and provide a more public API that inspector can switch over to. I'd be happy to do the PR work on inspector as well if this sounds acceptable. Otherwise I'm open to suggestions..

@teddyzeenny
Copy link
Contributor

We can easily update the inspector (it should keep supporting the old version though). I'm more worried about Ember.libraries being a public API, and Ember Inspector is just one of many libraries and apps using it. As @jayphelps suggested, mocking the array-like behavior might be a good approach.

@jayphelps
Copy link
Contributor Author

So basically....I've been banging my head the past couple days trying to actually implement these changes in a way that A. won't break old Ember Inspector's, B. preferably not break third parties using the existing array-like behavior.

I tried mocking the array-like behavior and quickly found existing Ember Inspector doesn't just expect it to have array-like methods, it also expects it to basically be an array. I couldn't find a way to mock the array-like behavior in a way that Array.prototype.slice(Ember.libraries) would still work as expected. (please correct me if someone knows the magic way)

We can't actually extend the Array.prototype and maintain all the array-like behavior either since browser support for this is terrible and not worth it.

SO...now I basically came full circle. I don't think there's anything that can be done about this without breaking old inspectors and third party code that relies on this. I'm stuck, so if anyone has a solution please let me know. Maybe there's some way we can do both the old way with a real array and a new way for a transitional period? That's the only way I know of.

At the very least, we can stop using the libraries.each helper, and deprecated it, which fixes #9339.

@teddyzeenny
Copy link
Contributor

@jayphelps thanks for spending time on this. There are no old versions of the inspector, it's always auto-updated to the latest version everywhere, so no need to worry about inspector issues, we'll just update it and publish.

The only thing to worry about is other libraries. If we can assume that they just use it to register themselves, then just keeping the method libraries.registerCoreLibrary should be enough. Thoughts?

@jayphelps
Copy link
Contributor Author

@teddyzeenny If we're okay with that, then we're golden. AFAIK Chrome extensions don't apply the auto updates until the browser is restarted though, like Chrome itself? If we're OK with breaking that?

We can add a warning that they're likely using an outdated version of inspector, is there a way to access the inspector version number? I see it's injected into the inspectors container at config:main but I can't seem to find a path I can access it in the browser context.

@wagenet
Copy link
Member

wagenet commented Nov 1, 2014

@teddyzeenny @stefanpenner is this good to go?
@jayphelps can you squash your commits?

@stefanpenner
Copy link
Member

@teddyzeenny r?

@jayphelps
Copy link
Contributor Author

@wagenet I was hoping to get some feedback from @teddyzeenny. (See my last comment)

This PR is NOT ready, but I'll go ahead and guess what he'll say and go off that.

@teddyzeenny
Copy link
Contributor

@jayphelps let's not worry about the inspector. I'm always working under the assumption that everyone has the latest inspector version.

@rwjblue
Copy link
Member

rwjblue commented Nov 8, 2014

@jayphelps - What is left here? If nothing, then it needs a rebase...

@rwjblue
Copy link
Member

rwjblue commented Nov 21, 2014

Will need a rebase.

@jayphelps
Copy link
Contributor Author

@rwjblue Rebased and AFAIK ready. On the inspector side, emberjs/ember-inspector#263 is needed.

Cc/ @teddyzeenny

@jayphelps jayphelps changed the title [WIP] Restructure Ember.libraries to be more idiomatic. Fixes #9291 Restructure Ember.libraries to be more idiomatic. Fixes #9291 Nov 21, 2014
@rwjblue
Copy link
Member

rwjblue commented Nov 21, 2014

Looks good to me. I think we need to merge and release emberjs/ember-inspector#263 before merging this (otherwise inspector blows up for canary apps).

@teddyzeenny - Can you let us know when we are good to merge here?

teddyzeenny added a commit to emberjs/ember-inspector that referenced this pull request Nov 21, 2014
Changes where we find the Ember.libraries array of registered libs that changed with emberjs/ember.js#9312
teddyzeenny pushed a commit to teddyzeenny/ember-inspector that referenced this pull request Nov 21, 2014
@teddyzeenny
Copy link
Contributor

New Ember Inspector version released. This can be merged.

@stefanpenner
Copy link
Member

👍 you are welcome to merge @teddyzeenny (i know you have commit bit)

teddyzeenny added a commit that referenced this pull request Nov 21, 2014
Restructure Ember.libraries to be more idiomatic. Fixes #9291
@teddyzeenny teddyzeenny merged commit 4ac66e1 into emberjs:master Nov 21, 2014
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.

5 participants