Suggestions: package improvements

marcalexiei commented 4 months ago

I would like to propose some improvements for this package:

Use `peerDependencies`

Add `rollup` to `peerDependencies`

Since this is a rollup plugin I think this should be added to peerDependencies. This will also help to clearly identify which rollup version are supported.

Add `sass` to `peerDependencies`

I think that user should install separately his sass implementation instead of having installed because is included already in dependencies. Also, like with rollup, we can clearly identify which version of sass are supported, if user install it.

Example:

{
  // ...
  "peerDependencies": {
    "sass": "^1.3.0",
  },
  "peerDependenciesMeta": {
    "sass": {
      "optional": true
    },
  },
  // ...
}

A working example is the webpack sass-loader package.json

Add other sass runtime

consider using webpack-loader approach so user can install only one sass runtime (right now sass will be always be installed since it is a dependency)

Code updates

Update source code using async functions and spread operator

I think that code could be updated using async functions in order to reduce callback chain code, additionally and use modern syntax like spread instead of or Object.assign and Array.prototype.concat.

These features are available for node >= 10

Enable `typescript` strict mode

Pretty self-explanatory 😅

[!NOTE] If you are ok with at least one of the proposed changes I can take care of doing separate PR's for each task This should ease review process a lot!

elycruz commented 4 months ago

These look good (👍). We can go ahead and open tickets for them - Also, in reply to your follow-up ticket - we should have separate tickets for them updates like these.

marcalexiei commented 4 months ago

Ok! Once we are happy with #139 I'll start to work on #141 tasks!

marcalexiei commented 1 month ago

Sass 1.79 has been released.

From the changelog:

While the legacy API has been deprecated since we released the modern API, we now emit warnings when the legacy API is used to make sure users are aware that it will be removed in Dart Sass 2.0.0. In the meantime, you can silence these warnings by passing legacy-js-api in silenceDeprecations when using the legacy API.

Right now this plugin is using the legacy API and the warning is present in the logs due to this sass change.

I would like to integrate the proposal done in "Add sass to peerDependencies" with the following points:

drop old versions of node, right node 10 is supported but I would support current LTS which is >= 18
dropping older version of node allow to convert this package to type: module and provide ESM code directly
add other sass runtime like sass-embedded to the peerDependencies
migrate code to use new sass API and get rid of the above warning

I'm available to make this changes after #141 is completed: I only need to add eslint and prettier which that shouldn't take long after #156 is merged.

elycruz / rollup-plugin-sass