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

Breaking change "/" as division with [email protected] #12233

Closed
xavierfoucrier opened this issue May 21, 2021 · 20 comments
Closed

Breaking change "/" as division with [email protected] #12233

xavierfoucrier opened this issue May 21, 2021 · 20 comments

Comments

@xavierfoucrier
Copy link

xavierfoucrier commented May 21, 2021

Using Foundation for sites 6.6.3 with sass 1.33.0 throw a lot of deprecation warnings in the console output.

image

See:

@xavierfoucrier
Copy link
Author

@DanielRuf, should I open a dedicated pull request for that to bump the current 6.6.3 version? Thanks!

@DanielRuf
Copy link
Contributor

@DanielRuf, should I open a dedicated pull request for that to bump the current 6.6.3 version? Thanks!

Which version do you want to bump? Afaik we don't use dart-sass in this project. I guess you mean a PR to migrate / update all the relevant code parts to use math.div(), correct?
If so, is this compatible with all previous Sass versions?

@xavierfoucrier
Copy link
Author

@DanielRuf Exactly. Sounds like the latest sass replace all the / divisions with math.div. I need to check if this is supported by old sass version. I will let you know soon. A bump version from 6.6.3 to 6.6.4 should be enought.

Typo about dart sass, sorry!

@DanielRuf
Copy link
Contributor

A bump version from 6.6.3 to 6.6.4 should be enought.

I'm not involved in Foundation Sites anymore so @joeworkman has to do this then.

@xavierfoucrier
Copy link
Author

xavierfoucrier commented May 27, 2021

Hey @DanielRuf, sad to hear 😿

It looks like math.div has been introduced in [email protected] as described here:

I am using Foundation in a webpack project that run sass (meaning "dart-sass", not "node-sass") for the compilation, and all the warnings comes from the fact that current [email protected] version is using "/" as a division operator.

Note that there is a sass migration tool to migrate.

cc @joeworkman

@DanielRuf
Copy link
Contributor

According to the note it is not supported by libsass / node-sass.
Not sure how many already use sass instead of libsass / node-sass.

https://www.npmjs.com/package/node-sass

Dart Sass
since 1.33.0
LibSass

Ruby Sass

That makes it hard(er) to support all users.
For me this looks like a major change as it is also documented as breaking change by the sass language team.

https://sass-lang.com/documentation/breaking-changes/slash-div

Personally I would not bump the patch version of Foundation Sites, because this could break many setups which use ~ or even ^ SemVer selectors for Foundation Sites.

@xavierfoucrier
Copy link
Author

Both node-sass and libsass are deprecated, see https://github.com/sass/node-sass#node-sass.
The core webpack team recommend using dart-sass for all new web projects since many years.

@pataar
Copy link

pataar commented May 27, 2021

An update to dart-sass did reduce the number of deprecation warnings by a lot. See: https://github.com/sass/dart-sass/releases/tag/1.34.0

Might this be something to upgrade in version 7? As that would be fitting because of the major upgrade.

gulp-sass does support dart-sass. Although you need some configuration changes and they still have node-sass as direct dependency in their package.json.

@xavierfoucrier
Copy link
Author

Hi @pataar! Yes, I am just discovering that, thanks 😉

But the fact is that it's not acceptable to work with those kind of deprecation warnings all the time.

Last Foundation version was released a year ago, and from my point of view it would be better to patch this kind of "easy issue" and move Foundation forward instead of accumulate a lot of issues/features that would increase the migration process later.

Having a 6.7.0 version should be better 🤔 ?

@pataar
Copy link

pataar commented May 27, 2021

Even though I'd like to have less deprecation warnings as well, it's a breaking change IMO. As certain versions and libraries would be suddenly incompatible. Which would require a major bump according to semver.

@xavierfoucrier
Copy link
Author

Ok so this should be part of Foundation v7 #11847, agree with that.

@katie1348
Copy link

I'm going to ask the obvious question, if this gets bumped to Foundation v7, what is the time frame for that release.

It doesn't look all that imminent. Is it likely to be this year?

Because having this SASS problem for another 6+ months is going to be a problem.

@xavierfoucrier
Copy link
Author

cc @joeworkman

@xavierfoucrier
Copy link
Author

Problem is that in that specific case, we are "stuck" to [email protected].* and can't update our version without having log output in the terminal. This is a problem when using tools like Github Actions where log output can be full of warnings.

Thanks for taking the time to give a clear answer to the community 😉

@katie1348
Copy link

For those running into this with dart SASS, there is a work around.

They have added a flag to suppress the deprecated warnings.

--quiet-deps

Add that to the command line and make sure that foundation is in the load-path of the command line. Any relative paths in your SASS will cause the warnings.

sass/dart-sass#672

This is a work around of course, it would be better to have a new version of Foundation in which this is not present.

@xavierfoucrier
Copy link
Author

xavierfoucrier commented Jun 4, 2021

@katie1348 I know about this flag, but it's a CLI flag, and I am running sass through a sass-loader with webpack, and there is not dart-sass javascript option at this time to quiet warnings.

See webpack-contrib/sass-loader#954.

@katie1348
Copy link

It's unfortunate, that there are multiple ways of running sass, but only one supports the flag.

I don't know of any other solution sorry.

I hope that this gets sorted out soon, as it is proving very disruptive and not being able to easily switch it off is a huge problem. I appreciate the need to provide warning about a breaking change, but this seems to be a break everyones workflow way of doing.

@joeworkman
Copy link
Member

Sorry for the delay. I was heads down getting a project shipped. Finally got it done last night. I think that @katie1348's suggestion is the best at this time. As I work on F7, I will ensure that the Sass side works perfectly with dart-sass. At this point, trying to get F6 warning free with dart-sass feels like a fruitless effort. I would rather put that time and energy into working on F7.

@xavierfoucrier
Copy link
Author

Thanks @joeworkman! 😎

I let you close this issue or pinned to Foundation v7 Github project.
Waiting for the next major release right now ☕ 😘

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

No branches or pull requests

5 participants