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

Suggestions: package improvements #140

Open
marcalexiei opened this issue Jun 29, 2024 · 3 comments
Open

Suggestions: package improvements #140

marcalexiei opened this issue Jun 29, 2024 · 3 comments

Comments

@marcalexiei
Copy link
Collaborator

marcalexiei commented Jun 29, 2024

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
Copy link
Owner

elycruz commented Jun 30, 2024

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
Copy link
Collaborator Author

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

@marcalexiei
Copy link
Collaborator Author

marcalexiei commented Sep 18, 2024

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.

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

No branches or pull requests

2 participants