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

Bad main #1

Merged
merged 1 commit into from
May 7, 2014
Merged

Bad main #1

merged 1 commit into from
May 7, 2014

Conversation

josh
Copy link

@josh josh commented May 7, 2014

@pennyfx the recent changes in 0.2.3 have a regression in the bower.json main. main must only be a single JS file (you can have alternative css files, but its not relative here).

This bootstrap file is basically useless, since its repeated here.

https://github.com/components/CustomElements/blob/0.2.3/src/custom-elements.js#L366-L368

See the 0.2.1 tag

https://github.com/components/CustomElements/blob/v0.2.1/bower.json#L8
https://github.com/components/CustomElements/blob/v0.2.1/custom-elements.js#L6

@pennyfx
Copy link
Member

pennyfx commented May 7, 2014

I didn't know there was a restriction on the number of files in main:, but I skimmed through bower/bower#393 and now understand the reasoning.

Nice to see that there is an if check inside the library for scope, so it doesn't die when window.CustomElements isn't defined.

Looks good. r+

pennyfx added a commit that referenced this pull request May 7, 2014
@pennyfx pennyfx merged commit 26233d0 into master May 7, 2014
@josh josh deleted the bower-main branch May 7, 2014 16:42
@josh
Copy link
Author

josh commented May 7, 2014

Should we just axe src/bootstrap.js from the checkin and build process?

@pennyfx
Copy link
Member

pennyfx commented May 7, 2014

I'm taking care of that now and I'll re-tag it.

@josh
Copy link
Author

josh commented May 7, 2014

Thanks!

@pennyfx
Copy link
Member

pennyfx commented May 7, 2014

I looked at the code closer and the window.CustomElements check still needs to happen. The first part of the polyfill is setting up the Observer and the scope that is passed in there is window.CustomElements. I didn't see any place where they ensure its defined before using it.

@pennyfx
Copy link
Member

pennyfx commented May 7, 2014

Yeah, polymers loader takes care if it here. https://github.com/Polymer/CustomElements/blob/master/custom-elements.js#L21.

I'll re-add bootstrap.js but make it part of the grunt:concat instead of main:.

@josh
Copy link
Author

josh commented May 7, 2014

The first part of the polyfill is setting up the Observer and the scope that is passed in there

For MutationObservers? Confused.

I didn't need to do anything in https://github.com/components/CustomElements/blob/v0.2.1/custom-elements.js.

This CustomElements v0.2.1 is whats shipped in production on github right now.

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.

2 participants