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

Remove node-sass in favor of dart-sass #64

Merged
merged 4 commits into from
Feb 25, 2021
Merged

Remove node-sass in favor of dart-sass #64

merged 4 commits into from
Feb 25, 2021

Conversation

Levdbas
Copy link
Contributor

@Levdbas Levdbas commented Feb 24, 2021

Since node-sass is now deprecated I made this new PR to further update this package.

  • I swapped out node-sass for dart-sass
  • Fixed a deprecation notice in the Buffer() usage
  • Removed the tests containing min()/max() since sass now uses the native css function now and thus the tests failed.

When running the tests a lot of errrors are spit about how the css is parsed but the tests run succesfully. We might want to look into this later on.

@civan
Copy link
Member

civan commented Feb 25, 2021

This is great thanks, I will update the Readme now it won't use node-sass, and then publish a new version, I will keep you posted! thanks again.

@civan civan merged commit b074dc9 into plentycode:master Feb 25, 2021
@civan
Copy link
Member

civan commented Feb 25, 2021

Hi @Levdbas changes are merge but test are failing even thought it says they passed, I will take a look into it and get back to you!

@Levdbas
Copy link
Contributor Author

Levdbas commented Feb 25, 2021

You think it's because of the map errors being spit out?

Error: (breakpoint: 320px, icon: "value") isn't a valid CSS value. ╷ 39 │ #sass-export-id.map-get{content:"#{( breakpoint: map-get($bps, 'mobile'), icon: map-get($icons, music) )}";}

@civan
Copy link
Member

civan commented Feb 25, 2021

yes, there is an issue open here sass/libsass#3048 and here sass/dart-sass#898 regarding this error

@Levdbas
Copy link
Contributor Author

Levdbas commented Feb 26, 2021

Hi @civan ,

I think that we cannot expect the libsass issue being resolved since the output is invalid css in the first place.

So I tried to come up with a way to work around this issue and tried several approaches.

  • Used the quote() function of sass to wrap the maps in quotes, no luck
  • tried to wrap the interpolated map in quotes, no lock.
  • combinations of the two, no luck.

But I found out when map values are handed off like this to rendersync

#sass-export-id.bps { 
  content: #{'("mobile": #{$bp-mobile}, "tablet": #{$bp-tablet}, "desktop": #{$bp-desktop}, "large-desktop": #{$bp-large-desktop})'};
}

the compiled css comes out like this which sass does accept as a valid css value.

#sass-export-id.bps {
  content: ("mobile": 320px, "tablet": 600px, "desktop": 1000px, "large-desktop": 1440px);
}

So there might be a way to work around this. Only thing that has to happen is to interpolate the scss variables/values inside a map in an earlier stage before they are handed of to the rendersync hook.

Is that something that might be possible you think?

@Levdbas
Copy link
Contributor Author

Levdbas commented Mar 22, 2021

Hi @civan ,

I did another attempt at cracking this issue. I came up with using https://sass-lang.com/documentation/modules/meta#inspect and we are now very close it seems. I hope you can help me with these last errors: See Levdbas@e7adbb2

Error 1
Seems to be related to the use of inspect(). Do you think it matters to the output when a quoted value is returned?

 1) Converter class
       function result support
         should return correct compiledValue when function is used:

      AssertionError: expected '"bootstrap/"' to equal 'bootstrap/'
      + expected - actual

      -"bootstrap/"
      +bootstrap/

Error 2
Same as above.

  2) Converter class
       map support
         should work with map-get within maps:

      AssertionError: expected '"value"' to equal 'value'
      + expected - actual

      -"value"
      +value

Error 3
I do think this is just a matter of updating the test am I right?

3) Utils class
       should wrap a variable:

      AssertionError: expected '#sass-export-id.var{content:"#{inspect($the-value)}";}' to equal '#sass-export-id.var{content:"#{$the-value}";}'
      + expected - actual

      -#sass-export-id.var{content:"#{inspect($the-value)}";}
      +#sass-export-id.var{content:"#{$the-value}";}

Error 4, only in console.
I am not able to tell where the error originates from. Maybe you can have a look?

 buffers support
    ✓ should resolve from buffer
error Cannot read property '0' of undefined
    ✓ should work from a file buffer (110ms)
    ✓ should return an array when the array option is specified
error expected 0 to equal 2

@Levdbas
Copy link
Contributor Author

Levdbas commented Mar 28, 2021

Hi @civan,

New PR is ready at: #67

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.

2 participants