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

add api option to allow switching to sass modern compiler #166

Merged
merged 14 commits into from
Nov 18, 2024

Conversation

marcalexiei
Copy link
Collaborator

@marcalexiei marcalexiei commented Oct 14, 2024

New api option

Add new optional api option to allow opt-in for sass modern compiler API.
Allowed values are:

  • legacy (defaultOne) to preserve backward compatibility (consider to change this in a new major)
  • modern uses compileStringAsync to compile sass code to css. The compile result transformations are the same of legacy option value.

Due to the new API structure the two syntax are handled inside a switch with two different blocks.

Code examples
// should default to legacy when `api` is not specified
sass({ options: { data: '', outputStyle: 'compact' } });

// should error when LegacyOptions are used with `api: 'modern'`
// @ts-expect-error
sass({ api: 'modern', options: { data: '', outputStyle: 'compact' } });

// should only accept api parameter
sass({ api: 'modern' });

// should correctly infer modern API options
sass({ api: 'modern', options: { style: 'compressed' } });

Refactors

In order to support the new syntax and retain backward compatibility different refactors have been made:

  • processRenderResponse has been moved in a separate file inside utils
  • getImporterList has been moved in a separate file inside utils and renamed to getImporterListLegacy, the same file houses the modern function variant: getImporterListModern
  • src/types
    • removed unused rollup types
    • removed sass types aliases, now sass related types are imported from sass
    • RollupPluginSassOptions now is a type due the new api option
      • relevant unit tests are added
    • PluginState has been renamed to RollupPluginSassState and moved here
  • src/utils/index content has been divided into 2 files with self-explanatory names:
    • src/utils/logger
    • src/utils/helpers
  • Plugin transform function now is an async function

Pending PR tasks

  • find a way to dedupe test code (use macro)
  • silence deprecation warning in legacy tests used sass 1.78 which doesn't have legacy-js-api warning
  • update README documentation
  • check logger function usages and consider to a prefix so user can clearly understand that they are coming from the plugin (e.g.: [rollup-plugin-sass]:) can be done in a separate PR

Follow up


@marcalexiei marcalexiei requested a review from elycruz as a code owner October 14, 2024 12:02
@marcalexiei marcalexiei marked this pull request as draft October 14, 2024 12:13
@marcalexiei

This comment was marked as outdated.

@marcalexiei marcalexiei force-pushed the new-compiler branch 4 times, most recently from ba8fdd7 to 7399ec1 Compare October 18, 2024 13:19
@marcalexiei marcalexiei force-pushed the new-compiler branch 2 times, most recently from f867454 to cd05bcd Compare October 31, 2024 05:06
@marcalexiei marcalexiei marked this pull request as ready for review October 31, 2024 05:08
@marcalexiei marcalexiei marked this pull request as draft October 31, 2024 07:08
@marcalexiei

This comment was marked as resolved.

@marcalexiei marcalexiei marked this pull request as ready for review November 1, 2024 03:58
@elycruz
Copy link
Owner

elycruz commented Nov 18, 2024

LG - merging for further review.

@elycruz elycruz merged commit 3008e00 into elycruz:main Nov 18, 2024
8 checks passed
@@ -219,63 +77,112 @@ export = function plugin(
}
},

transform(code, filePath) {
async transform(code, filePath) {
Copy link
Owner

Choose a reason for hiding this comment

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

Good job @marcalexiei - One thing - I noticed you're using async here - Since we're (currently) supporting node versions ">= 10" I'm not sure we can just assume that all clients have support for async/await - Seems we may have to release a new (major) version of the plugin that makes this change. Thoughts?

Copy link
Owner

Choose a reason for hiding this comment

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

Also, when I say "we're supporting..." I mean by way of our 'package.json' file.

Copy link
Collaborator Author

@marcalexiei marcalexiei Nov 18, 2024

Choose a reason for hiding this comment

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

From MDN: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function#browser_compatibility

Async functions are supported from Node version 7.6.0. so this shouldn't be a breaking change.
(I haven't tested it out 😅).

Anyway if we are releasing this as a breaking change I would consider to make api: modern the default value.


Created a new issue #173

Also I would like to do another change before the next release:
I'm trying to use processor to generate CSS modules:

    sass({
      processor: async (styles, filePath) => {
        let scopedClassNames = {};
        const postcssProcessResult = await postcss([
          postcssModules({
            getJSON: (_, json) => {
              if (json) scopedClassNames = json;
            },
          }),
        ]).process(styles, {
          from: filePath,
        });

        return { ...scopedClassNames, css: postcssProcessResult.css };
      },

Right now the plugin always returns the css string as export default and I haven't found a clean solution to change it that using processor options.
Have you implemented something similar that I can use or do you know another way?

My possible solution is to allow processor output a new property (e.g.: cssModuleMap) that, if present will be used as default export. What do you think?

If you want to discuss this further I will open a separate issue since it is out of scope in this PR 😁.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants