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

Move to Dart Sass compiler #25628

Merged
merged 2 commits into from
Oct 28, 2020
Merged

Conversation

yscik
Copy link
Contributor

@yscik yscik commented Sep 24, 2020

Fixes #22729

Description

  • Replaces node-sass with sass (Dart Sass) as the SASS compiler implementation. — This is a drop-in replacement with the same API, but without having to compile native C bindings.

  • Adds a patch for react-dates, renaming it's _datepicker.css file to the scss extension

    • This file is included by the DateTimePicker component
    • It has, for some reason, darken() function calls, which fails Dart Sass's CSS validation, as it's a Sass function
    • Changing it's file extension is the only way I found for it to be interpreted as Sass

How has this been tested?

Comparing the output of the compiled CSS files, there are only stylistic differences:

  • Whitespace and linebreak changes around block endings
  • Double quotes around attribute values in attribute selectors
  • Numeric precision (number of digits after decimals) — "Dart Sass defaults to a sufficiently high precision for all existing browsers"

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@yscik yscik changed the title Move to dart-sass compiler Move to Dart Sass compiler Sep 24, 2020
@ZebulanStanphill ZebulanStanphill added the [Type] Build Tooling Issues or PRs related to build tooling label Sep 25, 2020
@gziolo gziolo added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Oct 26, 2020
@gziolo
Copy link
Member

gziolo commented Oct 26, 2020

@yscik, thank you for opening this PR. Let's make sure it gets some proper review. I will raise it as a topic in the next weekly WordPress Core JS chat.

@nerrad
Copy link
Contributor

nerrad commented Oct 27, 2020

This was brought up in the weekly WordPress Core JS chat (signup needed) and in general there's no opposition to this change. Will still need an approval from a review.

@@ -0,0 +1,4 @@
diff --git a/node_modules/react-dates/lib/css/_datepicker.css b/node_modules/react-dates/lib/css/_datepicker.scss
Copy link
Member

Choose a reason for hiding this comment

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

I was wondering if there is a better way to fix the issue with react-dates. Would it be possible to whitelist somewhere in the webpack config instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I'm understanding the build process correctly, the packages are built outside of webpack with a direct call to sass.render, so webpack's loader config is not used here. Haven't found a way to configure this in Dart-Sass, the syntax is determined based on file extension.

Another approach that seems to work is creating a symlink in the date-time component:
_datepicker.scss -> ../../../../node_modules/react-dates/lib/css/_datepicker.css

Copy link
Member

Choose a reason for hiding this comment

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

@import "node_modules/react-dates/lib/css/_datepicker";

would it work if the extension was explicitly provided there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it's _datepicker.scss, it won't find the file, if it's _datepicker.css, the issue remains.

Copy link
Member

@gziolo gziolo Oct 27, 2020

Choose a reason for hiding this comment

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

This issue still exists in the latest version:

https://unpkg.com/browse/[email protected]/lib/css/_datepicker.css

Maybe, we should simply copy this file (in the version that Gutenberg uses) to the component's folder and import the local version :)

@yscik yscik force-pushed the change/sass-compiler branch from 2693f73 to 3804b0c Compare October 27, 2020 17:22
@yscik yscik force-pushed the change/sass-compiler branch from 3804b0c to bd38117 Compare October 27, 2020 17:25
@aristath
Copy link
Member

node-sass is now officially deprecated so that's one more reason to go through with this 👍

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Let's move forward with this one and figure out what to do about react-dates CSS file separately.

@gziolo gziolo merged commit 5060734 into WordPress:master Oct 28, 2020
@github-actions
Copy link

Congratulations on your first merged pull request, @yscik! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts:

https://profiles.wordpress.org/me/profile/edit/

And if you don't have a WordPress.org account, you can create one on this page:

https://login.wordpress.org/register

Kudos!

@gziolo
Copy link
Member

gziolo commented Oct 28, 2020

Opened #26534 with a proposal to copy SCSS file from react-dates to @wordpress/components.

@etoledom
Copy link
Contributor

This PR seems to be breaking some colors on mobile:
wordpress-mobile/gutenberg-mobile#2787 (comment)

I noticed that after merging this PR, the $gray-100 color defined on

$gray-100: #f0f0f0; // Used for light gray backgrounds.
takes preference and overwrites the one defined on When previous this merge commit, the opposite was true.

We would expect that *.native.scss would always overwrite their non native counterparts.
Any direction on how to go around this issue?
cc @hypest @ceyhun

@etoledom
Copy link
Contributor

We have found a plausible reason and a workaround to this issue here: #26898

But still we would like to to find a proper solution soon. _color.scss should not be loaded if we have a _color.native.scss defined.

Any idea or direction is welcomed 🙏
cc @yscik @gziolo

@yscik
Copy link
Contributor Author

yscik commented Nov 11, 2020

Looks like it's caused by this issue intentional feature: sass/dart-sass#574: import statements with a relative path are not passed to the importer function for resolution. (Also see the compatibility note in Sass docs)

That pretty much breaks the whole concept of being able to specify a .native.scss override. Is it a common pattern to use the overrides this way? I see most of them are imported from the .native.js files directly, which should still work.

It could be pretty tricky to work around this limitation. Might be possible to add another preprocessing step for resolving the URLs in import statements before passing the content over to the Sass renderer?

For the specific case of the colors.sccs override it sounds like changing the import in _variables.scss from @import './colors' to @import 'colors' could also work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scripts: Consider replacing node-sass with Dart SASS
6 participants