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

Updated libsass to master ~ 3.0rc1 #444

Closed
wants to merge 1 commit into from
Closed

Updated libsass to master ~ 3.0rc1 #444

wants to merge 1 commit into from

Conversation

andrew
Copy link
Contributor

@andrew andrew commented Oct 3, 2014

Updated libsass to current master, including Thar Be Dragons and added new files and config to bindings.gyp

Still need to get it building on Linux (and travis) and windows as well.

Sass-spec is passing on mac 474 passing (4s) but there's a load of updates there as well, will send a separate PR for that.

@nschonni
Copy link
Contributor

nschonni commented Oct 3, 2014

Might want to check because some of the old int returns are now C99 booleans

@HamptonMakes
Copy link
Member

We just released RC2. Just wanted to make you aware of some interface changes! See sass/libsass#505 for more info.

@am11
Copy link
Contributor

am11 commented Oct 6, 2014

I have upgraded to newest libsass and rewired bindings (to match the interface changes) and sass.js. The code is in my local test-libsass-05.10.2014 branch. There are 4 out of 474 tests failing, all related to source-map. Apparently, upstream isn't generating source-maps (for example map is null in the callback here). I tried to figure out without any luck so far. This discussion with @mgreter is relevant.

Also, libsass is now using C++11 features. So I have updated the .travis.yml. Node v0.10 exits gracefully (all green, just 4 tests are failing..) but v0.11 is throwing segmentation fault when it hits the first source-map (failing) test. See this travis-ci build.

You can clone my branch with: git clone --recursive https://github.com/am11/node-sass --branch=test-libsass-05.10.2014.

Few points:

  • Do we have only four tests for source-maps (where success callback has 2 parameters)?
  • Any pointers in that regard; pinpointing to the source of the problem will be quite useful?
    • Is it libsass or node-sass issue?
    • At this point, there are number of moving parts (flags) for source-maps alone. Anything can be blamed easily. So it needs real debugging for the failing tests.
    • I once configured VS debugger with node-gyp for node-sass. It took me sometime. Now I have a different machine and I forgot the configuration and doing have time to sort it out again (I should had written down a gist or blog for that..). Do you have other means to debug the whole stack? If so, please do it. :)

@michaek
Copy link

michaek commented Oct 6, 2014

We were uncertain whether it was "ok" or not to use C++11 features. Will that be that ok for node-sass? (I'm assuming it will, given your update to .travis.yml, but I wanted to check.)

I plan to look into the sourcemap support in node-sass/libsass. There are no tests for sourcemaps in sass-spec, so it's possible we introduced a regression we're unaware of in libsass.

@am11
Copy link
Contributor

am11 commented Oct 6, 2014

If you are referring to sourcemap mappings, you might need to take a look at this: sass/libsass#324. I can vouch for LESS' sourcemap to be the most complete/comprehensive sourcemaps for CSS superset (there a comparison with ruby-sass as well, lessc takes the lead there).

This issue I am having is with the source-map not being generated/returned from libsass. Some interface flag might be causing this. I don't know if other libsass' downstreams are testing whether source-map has generated at all.. Because for node-sass it is still failing four tests.

@am11
Copy link
Contributor

am11 commented Oct 7, 2014

It's FIXED!!! ㊗️

Had to change if(ctx->options.source_comments) { to if (ctx->source_map_string) { in binding.cpp.

Never give up is the way to go.. 😄

@am11
Copy link
Contributor

am11 commented Oct 7, 2014

@andrew, #448 is here for your review. :)

@laurentvd
Copy link

@am11, you are a hero ;-)

@andrew andrew closed this in #448 Oct 8, 2014
@andrew andrew deleted the libsass-3.0 branch December 13, 2014 17:28
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.

None yet

6 participants