Skip to content

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

@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