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

More closure fixes #1570

Merged
merged 7 commits into from
Sep 9, 2015
Merged

More closure fixes #1570

merged 7 commits into from
Sep 9, 2015

Conversation

sgomes
Copy link
Contributor

@sgomes sgomes commented Sep 8, 2015

This PR contains more fixes to help with Closure compiler support, adding fixes necessary to have it not only compile, but run correctly as well. This includes exporting symbols (using the foo['bar'] syntax), ensuring private properties aren't accessed by outsiders and typing things a bit more strongly in the componentHandler.

All unit tests pass on both compiled and uncompiled code.

@addyosmani @surma PTAL!

@Garbee
Copy link
Collaborator

Garbee commented Sep 8, 2015

LGTM. I don't see why any supported levels of interaction would break.

@surma
Copy link
Contributor

surma commented Sep 8, 2015

LGTM2 👍

mdlComponentHandler is ridiculous, though ^^

@sgomes
Copy link
Contributor Author

sgomes commented Sep 8, 2015

@surma Yup. It's more documentation than code, at this point...

@surma surma force-pushed the more-closure-fixes branch from 1f19446 to 26ea680 Compare September 8, 2015 22:40
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@googlebot googlebot added cla: no and removed cla: yes labels Sep 8, 2015
@surma surma force-pushed the more-closure-fixes branch from 26ea680 to 1f19446 Compare September 8, 2015 22:41
@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Sep 8, 2015
@surma
Copy link
Contributor

surma commented Sep 8, 2015

@sgomes I tried to rebase this against master to pull in @samccone fix (#1571) for uniffe() and remove the $.if, but I wasn’t sure I wouldn’t be screwing up the PR. It’s on the surma/more-closure-fixes branch.

@sgomes
Copy link
Contributor Author

sgomes commented Sep 9, 2015

@surma 🎶 Don't go breaking my branch... 🎤

Ahem. I don't think it's a problem to keep the $.if (and this way we'd keep the fixes separate), but if you feel strongly about this let me know and I'll pull in your branch changes :)

@sgomes
Copy link
Contributor Author

sgomes commented Sep 9, 2015

Actually, given that @samccone's fix is only for master, I'm inclined to merge this as-is. You ok with that, @surma?

@samthor
Copy link
Contributor

samthor commented Sep 9, 2015

btw, I'm glad this works, but it's clear each export becomes frustratingly verbose. We can probably export everything by default (which is fine, since everything's in self-evaluating etc etc) or use @export.

@samthor
Copy link
Contributor

samthor commented Sep 9, 2015

I'm happy to look more at this within the next ~24 hours (timezones are hard).

@surma
Copy link
Contributor

surma commented Sep 9, 2015

@sgomes Yeah, of course. Go ahead :)

@sgomes
Copy link
Contributor Author

sgomes commented Sep 9, 2015

@samthor I don't think there's a good way to export everything by default, other than using externs. In any case, I'll merge this as-is for now, and we can treat improvements as a new PR, so that we can do further development on Closure gulp tasks and the like in the meantime.

sgomes added a commit that referenced this pull request Sep 9, 2015
@sgomes sgomes merged commit 74bbd45 into mdl-1.0 Sep 9, 2015
@sgomes sgomes deleted the more-closure-fixes branch September 9, 2015 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants