Update meow dependency #41

ericcornelissen · 2021-10-31T23:38:44Z

No description provided.

Update the source code from CommonJS to ES modules style. This consists of changing `require`s to `import from` statements, `module.exports` to `export` statements, and in some cases the structure of imports and exports as well. Notably when the old `require` statement also contained some additional logic, which is not possible with import statements.

Update the meow dependency to ^10.1.1 - which is the latest version - and update how it is used in accordance with breaking changes.

The migration to ES modules makes it more complicated to import config files. Most notably, config files can no longer just be `require`d. So, to read and parse config files, and to support ES modules and CommonJS config files, cosmiconfig was added to load config files.

Update the source to remove remaining `module.exports` reference (in parse.js). Update the source to be able to load rules as ESM using `await import(...)` (in rule-loader.js), update dependent code accordingly to await that import (see svglint.js). Two tests are now marked as skipped, initial investigation suggests the emitting of the "done" event at linting.js:79 doesn't work well in combination with async/await.

package.json

Update ESLint to the latest major version for easier support of a config file with an `.cjs` extension as well as better support for ESM. Fix all the linting warnings/errors.

ericcornelissen · 2021-12-03T20:18:49Z

The current status of this Pull Request:

Refactored the source code from CommonJS to ESModules.
Updated Meow to a recent version.
The CLI is unchanged except for the fact that it now has a higher minimum NodeJS version requirement due to the source code being ESM.
The JavaScript API has changed from sync to async due to this change. In short, dynamically importing ESM appears to require async. Hence, to support ESM plugins it appears async is required.
Since the previous points already require a major version bump, I did not yet bother to look into providing a CommonJS version of either the CLI or JS API. Especially considering the fact that a transpiled CommonJS version of SVGLint would not be able to use ESM plugins, so from that viewpoint I'm also not sure if we even want to provide that option. EDIT: see this comment for an update.
It addresses Consider replacing meow dependency #34, Support ES Module configuration files #36, and likely Boolean options are parsed as arguments in CLI #44.

This means that a major version bump would be required to release this - for the NodeJS version requirement and the lack of CJS support and the JS API change.

@birjolaxew do you have any opinion on this? At the moment my opinion is leaning towards this solution - a mostly ESM-only solution with backwards compatibility support for CJS - because 1) it's simpler to implement and 2) it avoids any confusion due to someone trying to import an ESM config/plugin from the transpiled version of SVGLint.

If you disagree, there seem to be libraries to de-async functions which could help address the JS API changes and I can still try to look into transpiling to CJS.

Also, a correction to what I said before: not all v12 release of NodeJS support ESM, I believe (based on this) the first version with proper support is v12.20.0 (the latest v12, as of this comment, is v12.22.7).

Change from the approach in 033d7db and replace the try-catch technique of importing by just using `await import`. From some local testing it seems only that one is ever used, making the catch branch effectively dead code. Additionally, this fixes a bug introduced in 1e4eb07 where CJS config files were imported but ignored since CJS is always `import`ed on the default property.

LitoMore · 2021-12-03T20:49:20Z

The first version with proper support of ESM is v12.17.0. But meow and most of Sindre's packages are using node: protocol for imports, which is requiring v12.20.0.

Here is the discussion: sindresorhus/meta#15 (comment).

.github/workflows/test.yml

As 12.17 is the minimum required version for ESM support. 12.20 is the minimum required version if you want to use the `node:` protocol [1], which this project doesn't at the moment. -- 1. https://nodejs.org/api/esm.html#node-imports

package.json

.github/workflows/test.yml

ericcornelissen · 2021-12-12T18:02:21Z

I unfortunately don't know enough about CJS/ESM support to really weigh in on this (nor the time to read up on it). Although breaking changes (especially when it comes to dropping support for Node versions) are frustrating, I trust that the pros outweighs the cons if that's the general opinion. I'd be happy with this solution.

No problem and thanks for the reply nonetheless, I was mainly interested in if you're okay with the major version change for this reason. I'll be doing some further testing to decrease the chances of any breaking changes affecting users that are using a compatible NodeJS version

Not sure if the rules should be named exports, but there are a few other places too.

Thanks for the feedback @sachinraja - I agree some of these are better as named exports, however my aim with this PR is to be a straightforward translation from CJS to ESM (+necessary changes to keep things working AND updating meow). Since CJS module.exports are equivalent to ESM default exports I stuck with that. Given there's already 4 things in the scope of this Pull Request I intend to exclude the refactoring w.r.t. export/import styles.

sachinraja · 2021-12-14T00:12:04Z

Given there's already 4 things in the scope of this Pull Request I intend to exclude the refactoring w.r.t. export/import styles.

This looks good to me then 👍

Use the `"file://"` prefix when loading CLI config and rules to ensure importing always works on Windows.

Add a CommonJS version of the Node API of SVGLint by building it using Rollup [1]. Update the package manifest to allow for importing/requiring ESM and CJS (resp.) and `.gitignore` to ignore the generated CJS file. One "custom" Rollup plugin (provided by Rollup but not included by default) is used, namely `plugin-dynamic-import-vars` [2]. This plugin ensures that dynamic imports are properly handled as the CJS output generated by Rollup cannot dynamically import ESM rules. To make this work the rule-loader had to be updated as well, unfortunately the requirements of this plugin are quite strict and it is required that the `"../rules"` path is hardcoded into the `import` statement. Furthermore it has trouble detecting things that should be ignored, so errors are disabled to prevent errors due to dynamic imports of external plugins. -- 1. https://rollupjs.org/guide/en/ 2. https://www.npmjs.com/package/@rollup/plugin-dynamic-import-vars

ericcornelissen · 2021-12-14T21:54:30Z

I ended up trying to provide a CJS version of the API and I think I succeeded with f3e91a1. As a result this would now only be a breaking changes w.r.t.:

The minimum NodeJS version required to use the CLI.
The API, which went from sync to async.

On the positive side, this would remove the need to migrate for API users. On the negative side, going full ESM in the future will require an additional major version bump (whether that's because we want to or because a not-CLI-only dependency goes ESM only).

I'm personally of the opinion that we should go with the CJS support for now so that migrating is as easy as possible now and that the additional major version bump in the future isn't a huge deal. Does anyone in this thread have a different opinion?

Additionally, I would appreciate if anyone who has time could try these changes out locally (just copy this repo into node_modules) and report back any problems or successes + your Node version and Operating System. 🙂

sachinraja · 2021-12-15T04:41:53Z

Just cloned locally and ran node bin/cli.js icon.svg and it worked for node 16, latest node 14 (14.18.2), and latest node 12. But I did get this error with node 14.0.0 <= 14.8.0 < 14.13.1:

internal/process/esm_loader.js:74
    internalBinding('errors').triggerUncaughtException(
                              ^

Error [ERR_UNSUPPORTED_ESM_URL_SCHEME]: Only file and data URLs are supported by the default ESM loader
    at Loader.defaultResolve [as _resolve] (internal/modules/esm/resolve.js:781:11)
    at Loader.resolve (internal/modules/esm/loader.js:85:40)
    at Loader.getModuleJob (internal/modules/esm/loader.js:229:28)
    at ModuleWrap.<anonymous> (internal/modules/esm/module_job.js:51:40)
    at link (internal/modules/esm/module_job.js:50:36) {
  code: 'ERR_UNSUPPORTED_ESM_URL_SCHEME'
}

LitoMore · 2021-12-15T12:25:08Z

@sachinraja ~~You should not use node bin/cli.js to run the bin file.~~ Please use these commands below to test:

npm link
svglint icon.svg

# Once you want to unlink
npm un -g svglint

Or just run ./bin/cli.js icons.svg directly.

LitoMore · 2021-12-15T12:33:37Z

@ericcornelissen You could refer to this test if you want to test ESM behaviors.

Also we can use execa to test our bin-script. The code will be:

// test/cli.spec.js
import execa from 'execa';

const {stdout} = async execa('../bin/cli.js', ['--help']);
expect(stdout).toBe(helpText);

LitoMore

Looks good to me. It would be better if the test can be improved.

sachinraja · 2021-12-15T13:34:00Z

Or just run ./bin/cli.js icons.svg directly.

It shouldn't matter whether I run it with node or directly (that's all the shebang at the top of the file is doing anyway) but I get the same error with that. I also get it when I link the package.

LitoMore · 2021-12-15T13:44:35Z

It shouldn't matter whether I run it with node or directly.

Oh yes, you are right. I just missed some of your comments. I think you are using an unsupported Node.js version. Please refer to the engines field in package.json, we have already specified them in there. Users with an unsupported version will get a warning or error when they try to install our package.

FYI: https://docs.npmjs.com/cli/v8/configuring-npm/package-json#engines

For more ESM knowledge you can refer to https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c.

sachinraja · 2021-12-15T14:47:36Z

Please refer to the engines field in package.json, we have already specified them in there. Users with an unsupported version will get a warning or error when they try to install our package.

Ah you're right, didn't notice that change. npm doesn't seem to show the error when I just run npm install in the repo, but it does work when I try to install it into another project.

ericcornelissen · 2021-12-15T20:21:49Z

Thanks so much for all the help @LitoMore and @sachinraja! 😄

Looks good to me. It would be better if the test can be improved.

I agree they can be improved in the way you suggested, but, once again, I'd like to keep that for a later PR. For now I just want to be sure that running the CLI under normal circumstances on all major OSes and with all supported NodeJS versions works - i.e. doesn't error - I don't care so much about the actual output yet. It's currently just a smoke test.

Also, just to be explicit about it, running with the --help or --version flag would not be a sufficient test as the CLI exists early with those flags. So, while it would be good to test that the CLI works with those flags, it's more important to run the CLI with (and without) a configuration file and SVGs to lint.

Ah you're right, didn't notice that change. npm doesn't seem to show the error when I just run npm install in the repo, but it does work when I try to install it into another project.

I added .npmrc in 047242b to prevent confusion like this in the future. Thanks for helping to figure out the problem @LitoMore 😃

LitoMore · 2021-12-16T02:53:55Z

Also, just to be explicit about it, running with the --help or --version flag would not be a sufficient test as the CLI exists early with those flags.

I just give a simple demo to show how to use execa. 😄

ericcornelissen · 2021-12-18T22:08:50Z

Thanks once again @LitoMore and @sachinraja for feedback and testing 😃

github-actions · 2021-12-19T14:08:23Z

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

ericcornelissen added 3 commits November 1, 2021 00:32

chore(deps): update meow from ^3.7.0 to ^10.1.1

933cf72

Update the meow dependency to ^10.1.1 - which is the latest version - and update how it is used in accordance with breaking changes.

mondeja mentioned this pull request Nov 2, 2021

Fix require path error in examples #43

Merged

ericcornelissen mentioned this pull request Nov 2, 2021

Boolean options are parsed as arguments in CLI #44

Closed

ericcornelissen added the dependencies Pull requests that update a dependency file label Nov 2, 2021

ericcornelissen added 3 commits November 3, 2021 23:12

Merge branch 'master' into update-meow

d53539d

refactor: update tests to ES modules

85a26d4

LitoMore reviewed Nov 19, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

ericcornelissen added 6 commits December 3, 2021 18:47

Merge branch 'master' into update-meow

dd3e2c5

chore: update ESLint and fix linting errors

7a8dc5e

Update ESLint to the latest major version for easier support of a config file with an `.cjs` extension as well as better support for ESM. Fix all the linting warnings/errors.

chore: update package manifest for ESM only

35a02af

chore: unskip tests

2349787

docs: update JS API example

71b30f4

fix: support ESM and CJS config files

1e4eb07

ericcornelissen added 3 commits December 3, 2021 21:32

docs: update config example to use ESM syntax

e8db954

ci: run tests on Node v12, v14, and v16

31d582f

ericcornelissen marked this pull request as ready for review December 3, 2021 20:43

LitoMore reviewed Dec 3, 2021

View reviewed changes

.github/workflows/test.yml Show resolved Hide resolved

ericcornelissen added 2 commits December 4, 2021 12:15

chore: adjust minimum v12 version to 12.17.0

404fa2c

As 12.17 is the minimum required version for ESM support. 12.20 is the minimum required version if you want to use the `node:` protocol [1], which this project doesn't at the moment. -- 1. https://nodejs.org/api/esm.html#node-imports

ci: run tests on specific NodeJS versions rather then latest 12/14/16

6c8d97f

LitoMore reviewed Dec 4, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

revert: 404fa2c

8db8147

LitoMore reviewed Dec 4, 2021

View reviewed changes

.github/workflows/test.yml Outdated Show resolved Hide resolved

LitoMore mentioned this pull request Dec 4, 2021

convert scripts to esm simple-icons/simple-icons#6946

Merged

chore: remove unnecessary semicolons

bf07134

sachinraja approved these changes Dec 14, 2021

View reviewed changes

ericcornelissen added 3 commits December 14, 2021 21:17

fix: loading config files and built-in rules on Windows

6980c10

Use the `"file://"` prefix when loading CLI config and rules to ensure importing always works on Windows.

ci: run CLI on various node versions and operating systems

b67761b

LitoMore approved these changes Dec 15, 2021

View reviewed changes

chore: add .npmrc to enforce engine-strict option

047242b

LitoMore approved these changes Dec 16, 2021

View reviewed changes

sachinraja approved these changes Dec 16, 2021

View reviewed changes

ericcornelissen merged commit 2d242fe into simple-icons:master Dec 18, 2021

ericcornelissen deleted the update-meow branch December 18, 2021 22:07

github-actions bot added the released label Dec 19, 2021

ericcornelissen mentioned this pull request Dec 19, 2021

Add test suite for the CLI #49

Merged

ericcornelissen mentioned this pull request Jan 15, 2022

Support ES Module configuration files #36

Closed

ericcornelissen mentioned this pull request Oct 5, 2022

refactor: address default import #59

Merged

mondeja mentioned this pull request Oct 20, 2022

Use YAML format for eslintrc config file #65

Merged

ericcornelissen mentioned this pull request Jun 28, 2023

Use Node.js v20 in continuous integration #87

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update meow dependency #41

Update meow dependency #41

ericcornelissen commented Oct 31, 2021

This comment has been minimized.

ericcornelissen commented Dec 3, 2021 •

edited

Loading

LitoMore commented Dec 3, 2021

ericcornelissen commented Dec 12, 2021

sachinraja commented Dec 14, 2021

ericcornelissen commented Dec 14, 2021 •

edited

Loading

sachinraja commented Dec 15, 2021 •

edited

Loading

LitoMore commented Dec 15, 2021 •

edited

Loading

LitoMore commented Dec 15, 2021 •

edited

Loading

LitoMore left a comment

sachinraja commented Dec 15, 2021 •

edited

Loading

LitoMore commented Dec 15, 2021 •

edited

Loading

sachinraja commented Dec 15, 2021

ericcornelissen commented Dec 15, 2021

LitoMore commented Dec 16, 2021

ericcornelissen commented Dec 18, 2021 •

edited

Loading

github-actions bot commented Dec 19, 2021

Update meow dependency #41

Update meow dependency #41

Conversation

ericcornelissen commented Oct 31, 2021

This comment has been minimized.

ericcornelissen commented Dec 3, 2021 • edited Loading

LitoMore commented Dec 3, 2021

ericcornelissen commented Dec 12, 2021

sachinraja commented Dec 14, 2021

ericcornelissen commented Dec 14, 2021 • edited Loading

sachinraja commented Dec 15, 2021 • edited Loading

LitoMore commented Dec 15, 2021 • edited Loading

LitoMore commented Dec 15, 2021 • edited Loading

LitoMore left a comment

Choose a reason for hiding this comment

sachinraja commented Dec 15, 2021 • edited Loading

LitoMore commented Dec 15, 2021 • edited Loading

sachinraja commented Dec 15, 2021

ericcornelissen commented Dec 15, 2021

LitoMore commented Dec 16, 2021

ericcornelissen commented Dec 18, 2021 • edited Loading

github-actions bot commented Dec 19, 2021

ericcornelissen commented Dec 3, 2021 •

edited

Loading

ericcornelissen commented Dec 14, 2021 •

edited

Loading

sachinraja commented Dec 15, 2021 •

edited

Loading

LitoMore commented Dec 15, 2021 •

edited

Loading

LitoMore commented Dec 15, 2021 •

edited

Loading

sachinraja commented Dec 15, 2021 •

edited

Loading

LitoMore commented Dec 15, 2021 •

edited

Loading

ericcornelissen commented Dec 18, 2021 •

edited

Loading