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

Bump LibSass to 3.5.5 #2543

Merged
merged 5 commits into from Nov 12, 2018
Merged

Bump LibSass to 3.5.5 #2543

merged 5 commits into from Nov 12, 2018

Conversation

xzyfer
Copy link
Contributor

@xzyfer xzyfer commented Nov 11, 2018

  • update to LibSass 3.5.5
  • replace committed src/libsass with a git subtree
  • replace npm versioned sass-spec devDependency with a git hash

Fixes #2362

@xzyfer xzyfer self-assigned this Nov 11, 2018
@xzyfer xzyfer requested a review from nschonni November 11, 2018 05:55
Copy link
Contributor

@nschonni nschonni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this go against v5 branch?

@@ -47,7 +47,7 @@ int base64_encode_block(const char* plaintext_in, int length_in, char* code_out,
*codechar++ = base64_encode_value(result);
result = (fragment & 0x003) << 4;
#ifndef _MSC_VER
__attribute__ ((fallthrough));
/* fall through */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an upstream thing, but it seems odd to have a compiler directive for only a comment now

@@ -126,12 +121,11 @@ namespace Sass {

namespace File {

static std::vector<std::string> defaultExtensions = { ".scss", ".sass" };
static std::vector<std::string> defaultExtensions = { ".scss", ".sass", ".css" };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems off, is this the think where libsass was treating plain CSS as SCSS?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the revert of the CSS import deprecation warning as discussed in #2362

@xzyfer
Copy link
Contributor Author

xzyfer commented Nov 11, 2018

This LibSass version bump in specifically to resolve #2362 so it needs to land in v4.

Copy link
Contributor

@nschonni nschonni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think most of my comments here are for minor upstream things. I guess the idea is to get this in for a 4.12 before cutting a 5

@xzyfer xzyfer merged commit 7929c32 into master Nov 12, 2018
@nschonni nschonni deleted the libsass-subtreee branch November 12, 2018 18:19
@raniesantos
Copy link

Is this a complete revert of the CSS importing behavior or are there minor changes on how it works behind the scenes?

@xzyfer
Copy link
Contributor Author

xzyfer commented Nov 14, 2018 via email

Copy link
Member

@saper saper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks to me "libsass" version key didn't get updated and the version "3.5.4" got embedded in the binaries... causing #2621

jiongle1 pushed a commit to scantist-ossops-m2/node-sass that referenced this pull request Apr 7, 2024
…indicator

Make error indicator Unicode aware
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants