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

Update gulp-sass version. #1126

Closed
wants to merge 3 commits into from
Closed

Update gulp-sass version. #1126

wants to merge 3 commits into from

Conversation

Garbee
Copy link
Collaborator

@Garbee Garbee commented Jul 20, 2015

Upgrading sass builder to try and fix some reports of builds breaking. Issue #1118 for instance.

@addyosmani
Copy link
Contributor

@zeakd could you try this out?

@Garbee
Copy link
Collaborator Author

Garbee commented Jul 20, 2015

@addyosmani From their report:

I changed to same gulp-sass version of your project ( ^2.0.4 -> ^1.3.3 ) and it works.

I just changed from ^ on the start to an * on the patch to make sure we only update within patch releases. Minor updates we should re-assess in case of accidental breakage.

@addyosmani
Copy link
Contributor

@Garbee Thanks. I missed that in the original report. Manually verifying locally.

@Garbee
Copy link
Collaborator Author

Garbee commented Jul 20, 2015

OH wait, I misread the arrow direction.

They are using 2.0.4 in their project and it was breaking on them. #1075 may have fixed the problems with 2.0.4, so the master will work but 1.0.0 tag still won't.

Either way though, if 2.x works now we should go with it since it does lead to a faster build (plus new goodies.)

@addyosmani
Copy link
Contributor

Works for me locally. Regarding the tag: we're hopefully going to tag and publish 1.0.1 this week. @surma can we get a second verification from you that this doesn't cause any breakage?

@surma
Copy link
Contributor

surma commented Jul 20, 2015

On it

@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.

@surma surma force-pushed the upgrade-node-sass branch from e029e2a to 08b762b Compare July 20, 2015 14:42
@surma
Copy link
Contributor

surma commented Jul 20, 2015

npm install on node 0.12 only worked for me after I added phantomjs explicitly to package.json. Added a commit for that.

Microsite, components and templates look fine.

We are breaking node 0.10 btw, which I think we used to platform on.

$ gulp serve
...
Error: `libsass` bindings not found in /Users/surma/src/github.com/google/Material-Design-Lite/node_modules/gulp-sass/node_modules/node-sass/vendor/darwin-x64-14/binding.node. Try reinstalling `node-sass`?

Is node 0.10 still relevant? If not, LGTM.

@Garbee
Copy link
Collaborator Author

Garbee commented Jul 20, 2015

Ah, the phantom thing may have been what others were hitting as well. The install/updates worked for me (aside from once), then again I could have had a cache somewhere.

Node 0.10 is years old. Good to not support imo. Whether it is in LTS repos or not, NVM should be used to manage and keep node updated.

@Garbee
Copy link
Collaborator Author

Garbee commented Jul 20, 2015

P.S. Googlebot is drunk again.

@surma
Copy link
Contributor

surma commented Jul 20, 2015

Node 0.10 is years old. Good to not support imo.

Agreed. Good to merge, imo.

@Garbee
Copy link
Collaborator Author

Garbee commented Jul 20, 2015

Assigning to Addy for a quick review and merge of he agrees with us dropping 0.10 support.

@addyosmani
Copy link
Contributor

Current Node is 0.12.7 and 0.10.x is pretty old at this point. I did have a question around us opting for 2.0.* vs ^2.0.0, but @Garbee already included a comment about that. Before we merge this, two things:

@Garbee or @surma can either of you look at these items?

@Garbee
Copy link
Collaborator Author

Garbee commented Jul 21, 2015

I'll handle it. I can test windows 10 on actual hardware, 8.1 in a vm. A quick note on this should be simple as well.

@Garbee
Copy link
Collaborator Author

Garbee commented Jul 21, 2015

Just added a quick blockquote, "MDL requires NodeJS 0.12." underneath the "npm install" step of the instructions. Direct in point and focused where developers building will be looking.

@surma
Copy link
Contributor

surma commented Jul 21, 2015

👍

@Garbee
Copy link
Collaborator Author

Garbee commented Jul 21, 2015

FYI, found this issue on mocha-phantomjs's repo as to why the peerdep change occured. Maybe someone who knows that aspect better can look through real quick and see if there is anything we could improve with our package requirements around it.

@Garbee
Copy link
Collaborator Author

Garbee commented Jul 21, 2015

I'll dig into that issue actually. The phantomjs version specified is not satisfying the dependency.

And here we go down the rabbit hole!

@Garbee Garbee added cla: yes and removed cla: no labels Jul 21, 2015
@Garbee
Copy link
Collaborator Author

Garbee commented Jul 21, 2015

So, seems to work fully on Mac and Linux. It is Windows where we are having problems. But, those don't seem to be related to this update, but instead some phantomjs stuff that happened last week.

This patch functions and is good to merge. We should assess windows breakage on its own since that seems unrelated.

@surma
Copy link
Contributor

surma commented Jul 21, 2015

Agreed.

@addyosmani
Copy link
Contributor

I'm starting to get more direct reports of the PhantomJS issue arising. Looking at https://github.com/google/material-design-lite/commits/master/package.json, I can't see what could have caused this other than an upstream dep issue @Garbee mentioned.

Merging this and continuing to investigate issues with Phantom.

@Garbee Garbee closed this in 4d7e34e Jul 22, 2015
@Garbee Garbee deleted the upgrade-node-sass branch August 19, 2015 14:59
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.

4 participants