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

Updates SoyToIncrementalDomSrcCompiler #6

Merged
merged 3 commits into from
Apr 21, 2017

Conversation

jbalsas
Copy link
Contributor

@jbalsas jbalsas commented Mar 27, 2017

Hey guys!

This is an update of SoyToIncrDomCompiler to google/closure-templates@7dae5a1 that fixes #4 along with many other things.

/cc @mairatma, @yuchi

@jbalsas jbalsas changed the title Adds instructions to update SoyToIncrementalDomSrcCompiler Updates SoyToIncrementalDomSrcCompiler.jar Mar 27, 2017
@jbalsas jbalsas changed the title Updates SoyToIncrementalDomSrcCompiler.jar Updates SoyToIncrementalDomSrcCompiler Mar 27, 2017
@eduardolundgren
Copy link
Contributor

Hey @jbalsas did you run all metal and metal-components tests using this version?

@jbalsas
Copy link
Contributor Author

jbalsas commented Mar 27, 2017

I'm on it right now, was trying to figure out #7 before...

@brunobasto
Copy link

Hey @jbalsas, I see they changed the JS dependency too: google/closure-templates@7dae5a1#diff-68b90265f2e009fdb09b2c302a59838bL194

do we also need to update the JS deps we manually built from the previous version here: https://github.com/eduardolundgren/metal-soy-bundle ?

@jbalsas
Copy link
Contributor Author

jbalsas commented Mar 27, 2017

You're probably right, @brunobasto, I'm trying to set it up to run the tests on metal.js right now...

@jbalsas
Copy link
Contributor Author

jbalsas commented Mar 27, 2017

Yeah, we definitely need to update it, although the valid one is actually https://github.com/metal/metal.js/tree/master/packages/metal-soy-bundle

I'll take a look, but the changes are quite big...

@jbalsas jbalsas closed this Mar 27, 2017
@brunobasto
Copy link

@jbalsas Yeah, I now realized I posted the wrong link, sorry :)

Since we are talking about it.. It would save us a lot of time on the future if we could automate the process of creating that JS file. I tried that while ago using the closure compiler but, not only it didn't work, but the generated file was also a bit larger than the one we have now. I think @mairatma also tried that for some time.

@jbalsas
Copy link
Contributor Author

jbalsas commented Mar 27, 2017

Yeah, I was discussing it with @eduardolundgren right now... we should probably at least separate the things we're bringing in their different files, so diffing them in the future is easier, and the automate at least the concat and build part.

I'll ping you guys soon to discuss about it :)

@jbalsas jbalsas reopened this Mar 29, 2017
@jbalsas
Copy link
Contributor Author

jbalsas commented Mar 29, 2017

I'm reopening since this is actually all we need to support the new compiler... I've sent metal/metal.js#221 which should add support for it from metal-cli.

We can do a major version change here, to 4.0.0, and we'll only start using it in metal-cli when the updated soy-bundle is merged. That way we should be safe from regressions since no one would be picking these tools to compile unless ready.

@eduardolundgren
Copy link
Contributor

@jbalsas All tests are passing -- including all metal-components? If they are we are good to merge.

@jbalsas
Copy link
Contributor Author

jbalsas commented Apr 21, 2017 via email

@jbalsas
Copy link
Contributor Author

jbalsas commented Apr 21, 2017

Hey @eduardolundgren, ~~I didn't have much time today, but I've been able to test the following (with the following steps, which are not easy due to how everything is bundled).~~~

Below you can see the necessary steps and result of the tests of metal.js and metal-* with this compiler version.

TL;DR: Everything is green ☺️. Even if it weren't, we'll release a new major of metal-tools-soy, as well as a major of gulp-metal and metal-cli, so modules would need to opt-in into this making it less scary...

metal.js ✅

  • Delete node_modules/metal-tools-soy
  • Run npm link metal-tools-soy
  • Update gulpfile.js for the newer version

metal-components ☢️

  • Replace node_modules/metal-incremental-dom with the master version contents to fix Listener attributes not rendered properly metal.js#222. After this, tests are green. --> This is optional, depending if the tests are affected or not by it.
  • Delete node_modules/gulp-metal/node_modules/metal-tools-soy
  • Run npm link metal-tools-soy from node_modules/gulp-metal
  • Replace node_modules/metal-soy-bundle/build/bundle.s with the master version file
component tests
metal-alert
metal-autocomplete
metal-autocomplete-badges
metal-button-group
metal-datatable
metal-dropdown
metal-layout
metal-list
metal-modal
metal-pagination
metal-popover 🔴 (Tests already failing in metal-popover@master)
metal-progressbar
metal-rating
metal-reading-progress
metal-router
metal-select
metal-slider
metal-switcher
metal-tabs
metal-tooltip
metal-treeview

@eduardolundgren eduardolundgren merged commit 2e6c2e8 into master Apr 21, 2017
@eduardolundgren
Copy link
Contributor

@jbalsas Great news. Could you organize a minor release with those changes? Thanks!

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.

Doesn’t work correctly with files with windows-style line-endings (CRLF)
3 participants