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

Changed templating-tools to require uglify-js from NPM #234

Merged
merged 19 commits into from Feb 25, 2017

Conversation

@eagerestwolf
Copy link
Contributor

eagerestwolf commented Feb 22, 2017

I have been working with @abernix and @benjamn to replace uglify with the babili minifer in the minifier-js package to provide better ES6 support in Meteor (see meteor/meteor#8378 and meteor/meteor#8397). We have hit a roadblock, though, because the templating-tools tests are failing. I have made the changes suggested by @abernix that will make the code pass. The minifier is actually removing some of the newlines in the code, causing the tests to fail. This PR would fix that issue. I also had to update the version of node that Circle is using because Babili requires at least Node 4.

@eagerestwolf
Copy link
Contributor Author

eagerestwolf commented Feb 22, 2017

Ok, so over here I get the result that I should be getting in the Meteor repo. I think I know why though, for your tests you are pulling the package from Atmosphere, whereas over there it comes from the repo. Just a theory here.

@mitar
Copy link
Collaborator

mitar commented Feb 22, 2017

I am unsure why would system's node be important here? To my understanding Meteor should bundle NPM with it and have it used it to install Babili?

@benjamn
Copy link
Member

benjamn commented Feb 22, 2017

@mitar The Babili minifier won't run in Node versions older than 4.

@mitar
Copy link
Collaborator

mitar commented Feb 22, 2017

But why would system's node be used? I thought Meteor is not using it anymore?

@eagerestwolf
Copy link
Contributor Author

eagerestwolf commented Feb 23, 2017

@benjamn, I think @mitar is right here, Meteor doesn't use the system's version of node usually, in the Meteor repo itself, we have to use the correct version because we are building Meteor, but over here they aren't. They just download a release and use the devkit like most other people would. I will change that back.

@abernix
Copy link
Member

abernix commented Feb 23, 2017

I agree that changing the circle.yml Node version here is unnecessary and that can be changed back.

The Node version here is only used for the Hexo docs if I'm not mistaken. With that being said, I don't really see a harm in bumping the Node version for that to a more recent version. Regardless, that's a task for a separate PR/commit.

@eagerestwolf
Copy link
Contributor Author

eagerestwolf commented Feb 23, 2017

On a side note, that one test will continue to fail over here since Blaze does not use the meteor/devel version of minifier-js, but that change will make the meteor test pass so we can merge the change over there. I would update the dependency, but I am not sure if Meteor will pull the one from github or not.

@eagerestwolf
Copy link
Contributor Author

eagerestwolf commented Feb 23, 2017

I think I see where @mitar was coming from in asking. If something isn't broke it doesn't need fixing, and from what I can see, he is one of the main people over here in Blaze, so he knows better than anyone what changes need made.

@eagerestwolf
Copy link
Contributor Author

eagerestwolf commented Feb 23, 2017

I found our problem. Digging further into the Blaze code reveals that there is now a complete incompatibility between Blaze and Meteor because of this change. Blaze is using the UglifierJS beautify function, which Babili does not support, Babili is one way. So, instead of making their test pass, I think we should just have Blaze use uglify on their end as their own package.

@eagerestwolf
Copy link
Contributor Author

eagerestwolf commented Feb 23, 2017

That said, I can do one of two things for you guys, I can rewrite spacebars-compiler to use UglifyJS from npm, or I can create a new package exclusively for Blaze that exports UglifyJS, up to you @mitar, this is more your project.

@abernix
Copy link
Member

abernix commented Feb 23, 2017

We could consider just pinning minifier-js to the current version which uses UglifyJS in spacebars-compilers package.js:

  api.use('minifier-js@=1.2.17', ['server'], { weak: true });
@eagerestwolf
Copy link
Contributor Author

eagerestwolf commented Feb 23, 2017

That is a much better option than I came up with, I will update it and push it, I hate to have so many pushes, but my computer is sooo slow that tests take hours to run, even a single package. Gotta love rocking a single core AMD.

@abernix
Copy link
Member

abernix commented Feb 23, 2017

Not a huge deal, I don't think. Don't be afraid to push new commits either (instead of amending previous commits). It might make it a lot more clear how things have progressed. We can always squash at the end!

@eagerestwolf eagerestwolf changed the title Updated html-scanner-tests to work with new minifier Changed minifier-js to stay at 1.2.17 instead of Meteor's 2.0.0 Feb 23, 2017
@eagerestwolf
Copy link
Contributor Author

eagerestwolf commented Feb 23, 2017

I will make a push on my test branch of meteor and run my Travis build to see if it passes now, so I am not using MDG Travis resources.

@eagerestwolf
Copy link
Contributor Author

eagerestwolf commented Feb 23, 2017

Actually, I have just been rebasing and squashing as I go, to reduce the workload on you guys.

@eagerestwolf
Copy link
Contributor Author

eagerestwolf commented Feb 23, 2017

Alright! Blaze now passes, even though it fails because of my lack of the S3 keys to push the docs...you might wanna update that in the CI script, since passing CI is required to merge.

@eagerestwolf
Copy link
Contributor Author

eagerestwolf commented Feb 23, 2017

Ok, don't merge, meteor still won't pass with this change. Something is not right here, and I cannot peg it. If I run the test from the Blaze repo it passes, but not Meteor, I wonder if the problem is related to not having two versions of the same package installed at once. I need to modify the Travis script right quick to print the versions file to the console, and I will confirm whether or not that is the case.

@eagerestwolf
Copy link
Contributor Author

eagerestwolf commented Feb 23, 2017

Found the issue, you can only have 1 version of a package installed at a time, and the version of minifier-js that is installed is the new version, which is okay for the spacebars compiler because it has a failsafe in case it isn't there, but I am going to change the test right quick and you will understand everything, this will make both meteor and blaze pass all their tests.

@eagerestwolf
Copy link
Contributor Author

eagerestwolf commented Feb 23, 2017

Nope, still failing on meteor, I think the issue does go back to not having beautify available. I am going to try reworking the Blaze package to bring in uglify and see if that helps. Since Blaze isn't really working with ES6, they should be okay with uglify, and the code is going to be reminified with Meteor anyway later.

@eagerestwolf
Copy link
Contributor Author

eagerestwolf commented Feb 23, 2017

For those who are interested, the issue that is causing the test to fail is here.

@eagerestwolf
Copy link
Contributor Author

eagerestwolf commented Feb 23, 2017

So, now we have to figure out how to cope with this. minifer-js is no longer a weak dependency of spacebars-compiler, it now ships with uglify-js, if this update goes through. I am trying to think of alternate solutions, but for testing, I am seeing if this will make Blaze and Meteor pass all their tests.

Seth Murphy
@abernix
Copy link
Member

abernix commented Feb 23, 2017

I have just been rebasing and squashing as I go, to reduce the workload on you guys.

I meant you don't need to do that, and it might be helpful if you didn't. Squashing is one button at the end and it's actually nice to see what you've changed/tried in each commit to provide advice instead of trying to guess.

I am seeing if this will make Blaze and Meteor pass all their tests.

I think you actually have a circular dependency that may not be getting considered here and I don't know if you can actually fix the tests without . meteors repo has a submodule to the blaze repository which places it at packages/non-core/blaze. Typically, that repo points to the latest stable release of Blaze.

If you haven't changed that submodule reference to point to the modified Blaze branch with the fixed html-scanner-tests, you will have failures on the meteor branch.

I think the beautify issue is certainly something that needed to be addressed, but presumably the version pinning fixed it?

@eagerestwolf
Copy link
Contributor Author

eagerestwolf commented Feb 23, 2017

Ah okay, I will stop squashing my commits then. The thing is, I see why you say circular dependency, because spacebars-compiler is not seeing the version of minify-js that has the UglifyJS, but templating-tools is, and I think it is because for spacebars-compiler it is a weak dependency, whereas with templating-tools it is a strong dependency. As for the submodule, I updated it to use my git repo, while I am testing. I am honestly now sure why the output needs to be beautified anyway, but I don't really want to make a major change like that without permission.

Seth Murphy
@eagerestwolf
Copy link
Contributor Author

eagerestwolf commented Feb 23, 2017

Ok, so I keep getting this error and I cannot figure out why, I get the error if the folder is there, and I get it if it's not there. If I remove the .npm folder entirely, then meteor --get-ready just crashes.

spacebars-compiler: updating npm dependencies -- js-beautify...
/tmp/meteor/.meteor/packages/coffeescript/.1.11.1_4.1rnji0d++os+web.browser+web.cordova/plugin.compileCoffeescript.os/npm/node_modules/meteor/promise/node_modules/meteor-promise/promise_server.js:190
      throw error;
      ^

Error: ENOENT: no such file or directory, lstat '/home/ubuntu/blaze/packages/spacebars-compiler/.npm/package/node_modules'
Seth Murphy
@eagerestwolf
Copy link
Contributor Author

eagerestwolf commented Feb 24, 2017

Ok, given that I am having these issues with this. I have an idea, and I want MDG guidance. uglify-js is smaller than js-beautify by almost 100%. I can do 1 of 2 things in my mind, because this simply will not work. I can 1) create my own atmosphere package that does this, or 2) create a new package in Meteor Core that exports a beautify function. both are going to add some bloat to this package, ~1-2 MB, but if Blaze absolutely wants to keep the beautification, this is essential. For some reason, NPM packages won't install directly into here. @mitar your input is essential here, since you seem to be for the most part running Blaze.

@eagerestwolf
Copy link
Contributor Author

eagerestwolf commented Feb 25, 2017

Ok, so I found out why npm packages don't install over here. It turns out Blaze never goes through the build process like you guys do in the meteor repo. So, that is why it never finds the node_modules folder. It actually does find it, but it doesn't find its contents because they are never installed.

@eagerestwolf
Copy link
Contributor Author

eagerestwolf commented Feb 25, 2017

Nope wrong yet again. Turns out meteor is installing the npm dependencies, it just takes too long to complete. The timeout is 300ms, which is not long enough for meteor to install the npm packages. It might be best if we simply create a new package in meteor core for this. @abernix do you have any input on the issue?

@mitar
Copy link
Collaborator

mitar commented Feb 25, 2017

So I think we should depend on the NPM package. I do not know why this does not work.

@eagerestwolf
Copy link
Contributor Author

eagerestwolf commented Feb 25, 2017

@mitar It's the timeout, installing the npm packages causees the build to take more than 300 seconds. I'm testing a build config with a timeout of 30,000 seconds, also using my version of meteor, so I can remove as many variables as possible. I am still having an issue with meteor --get-ready dying unexpectedly. So I need to check the meteor logs to see what is causing that.

@eagerestwolf
Copy link
Contributor Author

eagerestwolf commented Feb 25, 2017

Or you know, I could just still be depending on a package that I didn't include in the shrinkwrap. That might just be it.

@eagerestwolf
Copy link
Contributor Author

eagerestwolf commented Feb 25, 2017

Also, in my circle.yml, I updated circle to only try to deploy the docs on tags, I think that might be a good option, or just simply having a different branch for your docs, that way circle tests will actually pass whenever someone tries to pull and you don't have to check circle manually to see if it was the docs that failed or something else.

@eagerestwolf
Copy link
Contributor Author

eagerestwolf commented Feb 25, 2017

Ok, so I ran an ssh on one of my builds and I am completely perplexed. The node_modules directory is there, so we can rule out the timeout issue. It seems like coffeescript is trying to run before the node_modules directory gets created. I am going to try to convert the coffee tests to pure javascript, commit that on master, and see what happens.

@eagerestwolf
Copy link
Contributor Author

eagerestwolf commented Feb 25, 2017

Ok, I have the problem pegged now, and I need MDG help to fix it. I have been testing the package locally, and what happens is while building the package, meteor actually installs uglify-js, then when it builds the web.browser target it deletes everything in the .npm folder, and I am not sure why. Do you have any suggestions @abernix ?

EDIT: Nevermind, I fixed it, turns out I need to Npm.depends() in Package.onTest() as well.

Seth Murphy
@eagerestwolf eagerestwolf changed the title Changed minifier-js to stay at 1.2.17 instead of Meteor's 2.0.0 Changed templating-tools to require uglify-js from NPM Feb 25, 2017
Seth Murphy added 2 commits Feb 25, 2017
@eagerestwolf
Copy link
Contributor Author

eagerestwolf commented Feb 25, 2017

Great news @mitar. I am done and will be out of your hair. All of the code is building correctly and all tests pass. You can merge whenever you want. Then we can update the reference in the meteor repo and meteor should build and pass all of its tests and we can close these PRs.

@eagerestwolf
Copy link
Contributor Author

eagerestwolf commented Feb 25, 2017

Ok, not done yet, now the blaze tests are failing. In particular video:


C: tinytest - blaze - render - video - autoplay : !!!!!!!!! FAIL !!!!!!!!!!!
{"groupPath":["tinytest","blaze","render","video"],"test":"autoplay","events":[{"sequence":10139,"type":"fail","details":{"type":"assert_equal","expected":"false","actual":"true","not":false},"cookie":{"name":"blaze - render - video - autoplay","offset":1,"groupPath":["tinytest","blaze","render","video"],"shortName":"autoplay"}}]}
C: tinytest - blaze - render - video - controls : !!!!!!!!! FAIL !!!!!!!!!!!
{"groupPath":["tinytest","blaze","render","video"],"test":"controls","events":[{"sequence":10142,"type":"fail","details":{"type":"assert_equal","expected":"false","actual":"true","not":false},"cookie":{"name":"blaze - render - video - controls","offset":1,"groupPath":["tinytest","blaze","render","video"],"shortName":"controls"}}]}
C: tinytest - blaze - render - video - loop : !!!!!!!!! FAIL !!!!!!!!!!!
{"groupPath":["tinytest","blaze","render","video"],"test":"loop","events":[{"sequence":10145,"type":"fail","details":{"type":"assert_equal","expected":"false","actual":"true","not":false},"cookie":{"name":"blaze - render - video - loop","offset":1,"groupPath":["tinytest","blaze","render","video"],"shortName":"loop"}}]}
C: tinytest - blaze - render - video - muted : !!!!!!!!! FAIL !!!!!!!!!!!
{"groupPath":["tinytest","blaze","render","video"],"test":"muted","events":[{"sequence":10147,"type":"fail","details":{"type":"assert_equal","expected":"false","not":false},"cookie":{"name":"blaze - render - video - muted","offset":0,"groupPath":["tinytest","blaze","render","video"],"shortName":"muted"}}]}
@mitar
Copy link
Collaborator

mitar commented Feb 25, 2017

Looks good. Thanks.

@mitar mitar merged commit 8558cb9 into meteor:master Feb 25, 2017
1 of 2 checks passed
1 of 2 checks passed
ci/circleci Your tests failed on CircleCI
Details
CLA Author has signed the Meteor CLA.
Details
@abernix
Copy link
Member

abernix commented Feb 26, 2017

@sethmurphy18 Your tests above seem to have failed spectacularly however the main repo CircleCI seems to be passing with those same commits merged: https://circleci.com/gh/meteor/blaze/tree/master. I don't actually see the specific video test failures you quoted above in the results though.

Anyhow, the Npm.depends should be at the top level of the package.js, not nested within Package.onUse and Package.onTest (though, things will work okay if they are identical in both). See meteor/meteor#8200 (comment) for a more complete explanation.

Things look okay other than that.

@mitar
Copy link
Collaborator

mitar commented Feb 26, 2017

Oh, I missed that. @abernix, thanks for catching that.

@sethmurphy18, could you make another pull request moving Npm.depends to the top?

@eagerestwolf
Copy link
Contributor Author

eagerestwolf commented Feb 26, 2017

Sure no problem.

@eagerestwolf
Copy link
Contributor Author

eagerestwolf commented Feb 26, 2017

We are also going to have to update the references to the minifier-js package eventually. It's going to version 2.0.0 which won't be picked up by the html-scanner package since only 1 version of a package can be installed at once and the version constraints won't pick up a new major version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.