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

Avoid dart-sass deprecation warnings #4725

Closed
BPScott opened this issue Nov 24, 2021 · 3 comments
Closed

Avoid dart-sass deprecation warnings #4725

BPScott opened this issue Nov 24, 2021 · 3 comments
Labels
Bug Something is broken and not working as intended in the system.

Comments

@BPScott
Copy link
Member

BPScott commented Nov 24, 2021

I've been starting to think about leveraging dart-sass. I've updated loom-plugin-build-extended to use dart-sass for package builds, and when attempting to use that in global-nav compiled output was as expected however it spammed the console with a bajillion deprecation notices regarding "use math.div instead of /" because using / to divide is deprecated in dart-sass.

Obviously this is a terrible user experience. It should be possible to use the public api with dart-sass without ruining console with incessant noise.

The gotcha comes here because polaris will need to support usage in consumers that use either node-sass or dart-sass.

Per my comments on #4298, fixes (as provided by the sass migrator) fall into one of two buckets:

  • A fix that compiles in both node-sass or dart-sass. Such as changing $x: 8 / 2; into $x: 8 * 0.5
  • A fix that keeps quiets dart-sass but is a compilation error in node-sass Such as changing $x: 8 / $var; into math.div(8, $var)

It should be easily possible to apply the fixes that compile in both straight onto main. But that leaves the other set.

Fortunately in the public-api the only cases where uncompilable fixes occur is in px(), rem(), em() and color-multiply() all of which are slated for removal in v9.

Options for that:

  • Do nothing, wait till v9 is released and deprecations go away (though there is no promise of new, different deprecate behavior creeping in later until at some point in the far future we drop support for consumers using node-sass)
  • After creating the build/styles folder, create a second build/styles-dart folder that contains the contents of src/styles, except ran through the sass migrator) - thus converting deprecated behaviour.
@BPScott BPScott added the Bug Something is broken and not working as intended in the system. label Nov 24, 2021
@BPScott
Copy link
Member Author

BPScott commented Jan 24, 2022

Command to see what files would throw deprecation warnings: npx sass-migrator division 'src/**/*.scss' --migrate-deps --load-path=. --dry-run

At time of writing, running this against the v9 branch shows the following files:

  • src/styles/shared/_breakpoints.scss. The one problem in our public API. Unfortunately its usage in consuming apps is pervasive. This isn't fixable till we rework how breakpoints are handled.
  • src/components/ColorPicker/ColorPicker.scss. This is a false positive - the slash is used to separate values, it's not an instance of division.
  • src/components/DatePicker/DatePicker.scss
  • src/components/Layout/Layout.scss
  • src/components/VideoThumbnail/VideoThumbnail.scss

@BPScott
Copy link
Member Author

BPScott commented Jan 29, 2022

Once the complete removal of the public sass api is done in #4980 and v9 is released there shall be no more public sass api, and thus we won't need to worry about writing mixins/functions that work regardless of if a consumer is using node-sass or dart-sass because we won't have any more consumers.

At that point we're free to either continue to use node-sass, or migrate dart-sass (by changing the compiler, and performing any migrations) at leisure.

@BPScott
Copy link
Member Author

BPScott commented Feb 22, 2022

Closing this. V9, which removes the public sass api, has shipped. No public sass api means no deprecation warnings when consumers try to use the public api.

At some point it'd be nice to migrate the polaris-react codebase to use dart-sass, but that work is now unblocked and can be done whenever with out affecting any consumers of the package.

@BPScott BPScott closed this as completed Feb 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken and not working as intended in the system.
Projects
None yet
Development

No branches or pull requests

1 participant