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

✨ feat: deprecation warnings for Less + monorepo chores #4319

Merged
merged 27 commits into from
Mar 1, 2025

Conversation

matthew-dean
Copy link
Member

What:

  • Adds deprecation warnings to Less output during parsing
  • Adds quiet option to silence warnings
  • Replaces Lerna w/ PNPM because I honestly can't remember how to use Lerna, and PNPM is easier
  • Adds contributor block to README.md
  • Cleans up some ESLint issues

Why:

  • It would be good to let people know of things that might are deprecated / discouraged and may be removed in the future
  • Preps for future Less versions

Checklist:

  • Documentation
  • Added/updated unit tests
  • Code complete

@dosubot dosubot bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label Feb 28, 2025
@iChenLei
Copy link
Member

Run actions/setup-node@v4
Attempting to download 14...
Acquiring 14.21.3 - x64 from https://github.com/actions/node-versions/releases/download/14.21.3-4202774076/node-14.21.3-linux-x64.tar.gz
Extracting ...
/usr/bin/tar xz --strip 1 --warning=no-unknown-keyword --overwrite -C /home/runner/work/_temp/c7e424d2-ee17-4303-ad5c-cd71ce6168e0 -f /home/runner/work/_temp/c488b341-208e-4f[10](https://github.com/less/less.js/actions/runs/13582507538/job/37970966845?pr=4319#step:4:11)-8bd6-e320e2bc1d13
Adding to the cache ...
Environment details
/home/runner/setup-pnpm/node_modules/.bin/pnpm store path --silent
ERROR: This version of pnpm requires at least Node.js v16.14
The current version of Node.js is v14.21.3

So just drop node 14 test ?

@matthew-dean
Copy link
Member Author

matthew-dean commented Feb 28, 2025

@iChenLei

So just drop node 14 test ?

Yeah, I think supporting Node versions 14-23 is unrealistic, especially if we're also trying to intersect with support on Windows / Mac. AND browsers. Node itself doesn't guarantee support that far back for its OWN versions.

So, maybe it should be like this:

Node support (Mac / Windows):

  • Maintenance LTS versions (18, 20)
  • LTS (22)
  • Current (23)

Browser support:

  • 1%. ?

It looks like we may be able to set this up in a maintainable way by using lts identifiers. In other words, I think our matrix could be ['lts/-2', 'lts/-1', 'lts/0', 'current'] ? But I'm not sure about those. The documentation is not clear, so they would need testing.

Btw, our current ci workflow has a lot of copy / paste. It can be simplified / reduced using a reusable workflow. You really only need one set of steps that defines setup / testing. Everything else should be configurable via matrices and parameters.

Also... 🤔 I think it's configured incorrectly, even beyond copy / paste. For instance, windows_and_macos_test and historical_versions_node_test say they need basic_node_test. As far as I know, that will run the basic_node_test before each of those. So I think there's a lot of duplication and redundant work here that can be cleaned up.

@matthew-dean
Copy link
Member Author

matthew-dean commented Feb 28, 2025

@iChenLei

Also... 🤔 I think it's configured incorrectly, even beyond copy / paste. For instance, windows_and_macos_test and historical_versions_node_test say they need basic_node_test. As far as I know, that will run the basic_node_test before each of those. So I think there's a lot of duplication and redundant work here that can be cleaned up.

Hey, I think I got this to work with dynamic versions! Check it out. I added one additional LTS version (v16 currently) for safety.

Oh I also fixed this:

Btw, our current ci workflow has a lot of copy / paste. It can be simplified / reduced using a reusable workflow. You really only need one set of steps that defines setup / testing. Everything else should be configurable via matrices and parameters.

There's now just one simple workflow.

@matthew-dean
Copy link
Member Author

@puckowski @iChenLei opinions on the deprecation notices in general?

@iChenLei
Copy link
Member

iChenLei commented Mar 1, 2025

Cool !

@matthew-dean matthew-dean merged commit d1abdab into less:master Mar 1, 2025
7 checks passed
@matthew-dean matthew-dean deleted the chore/add-deprecation-warnings branch March 1, 2025 19:23
@puckowski
Copy link
Contributor

Sorry, I did not have access to a working computer for a bit. I will still review this PR for any feedback/improvements even though it is already merged. @matthew-dean

@matthew-dean
Copy link
Member Author

@puckowski

Sorry, I did not have access to a working computer for a bit. I will still review this PR for any feedback/improvements even though it is already merged. @matthew-dean

I appreciate that! I wasn't going to create a release immediately in case you had a chance to look.

@puckowski
Copy link
Contributor

I tested these changes locally and things are working as expected so far.

I like the ---quiet flag and warning users about deprecations is a good change as we hope to streamline syntax.

Aside from a potential package.json Node version change (see comments) I see no critical changes required.

@matthew-dean

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants