solidjs-community / eslint-plugin-solid

Solid-specific linting rules for ESLint.
MIT License
220 stars 29 forks source link

Use a bundler or `package.json` exports to provide path exports (and modernize project) #147

Closed eagerestwolf closed 2 months ago

eagerestwolf commented 3 months ago

Describe the need So, as addressed (to some extent) in #137 and (for sure) in #146, the current setup for using the plugin is a bit confusing and a little obtuse. There are currently 2 ways to use the plugin (at least for ESLint 8/9), and neither is particularly great.

Obviously, the first way (I'll call the legacy way) is to import the plugin as shown below and call a flat config directly. This way of using the plugin seems to be discouraged though (despite working flawlessly....mostly), This is also the method that most plugin authors have settled on, since it requires basically no change to existing tooling.

Conversely, the recommended way to use the plugin doesn't work very well. If you include the .js extension that node requires to even load the module and not cause ESLint to error out, TypeScript will complain what is being imported is not a module and has no types. This is the exact issue that #146 is describing.

The reason for this discrepancy is related to how modern node handles modules. If your project is a modules project (i.e. "type": "module"), then .js extensions are required. It's complicated, but that's how things work now. Here's a page on the node docs if you want to read more. So why then is TypeScript complaining that what you just imported with that .js extension is not a module...because it's not. If you open the source .js file and look at it, it's not a module. It's CommonJS. Welcome to the wacky world of mixing ESModules and CommonJS.

Additionally, there are some other...issues...with the codebase. First, the .nvmrc calls for Node version 14.15.4, which has long been deprecated, and isn't supported by ESLint, Solid, or PNPM anymore (I suspect nobody tested the repo with nvm --use-on-cd). Officially, the lowest version of node supported by those 3 is 18.18.x. Next, there is still a peer dependency warning when installing eslint-plugin-solid with ESLint 9 because the version of @typescript-eslint/utils used (no harm there, the new version has only been out for 3 days). I'm not trying to be overly critical here, I'm just calling out all the issues I see, and DevOps is kind of my thing.

Suggested Solution From what I can tell, there are a few paths forward. I'm going to discuss what I think should happen here, and we can go from there (that's why I opened an issue and not a PR). I am willing to do all this work myself. I will walk through each change and explain why I think that's the best path forward. (NOTE: the changes I am going to suggest would normally result in a major version bump, but since this project is not 1.0 yet, it'll probably be a minor bump)

Possible Alternatives Technically, the standalone version could still exist, but shipping a patched version of ESLint seems like a bad idea. Additionally, I doubt it gets much use. In the same vain, it's technically possible to keep both forms of the flat configs, but it's probably best to pick one or the other and stick to it and most other plugins have settled on extending the existing plugin (I provide some examples in the additional context below). There is 1 notable exception I can think of...Prettier, but Prettier has kind of always done their own thing.

Additional context Plugins that are adding flat configs to the plugin itself (while these are cherry picked, they are cherry picked in that these are some of the more popular ESLint plugins):

pauliesnug commented 3 months ago

I think a mixture of those solutions is the best way to move forward. I doubt anyone is using the standalone version other than internally, so it can just be copied over into those internal plugins. Bumping typescript-eslint to v8 and the Node version makes sense, and there is already an easy PR for one of them. Tsup is a great bundler, which many plugins use.

pauliesnug commented 3 months ago

This would make developing the plugin easier and also remove any peer dependency warnings/TypeScript complaints (considering eslint.config.ts was added in the last ESLint update)

eagerestwolf commented 3 months ago

For sure. Using tsup would also remove the need for having a separate tsconfig.build.json since (for the most part) tsup ignores the tsconfig anyway. Having both CJS and ES exports would also help with the eslint.config.ts since TypeScript generally plays better with typed ES modules than typed CJS. I have started tinkering around a little bit, and the TSESLint references can be removed as that problem is fixed. Unfortunately, there is now a similar problem with scope-manager in the CompatContext. Sadly, I am not familiar enough with the internal workings of ESLint to truly understand what the CompatContext is doing.

I do have an idea that I think will strike a great balance here. What if we kept the concerns consolidated since the standalone linter is still relevant to the ESLint plugin, and just convert this into a monorepo with both eslint-plugin-solid and eslint-solid-standalone as packages in a packages directory. That way both projects can be updated in tandem since they both, to some extent, depend on each other.

One more suggestion that feels appropriate so as to avoid breaking things any more than is absolutely necessary. I have thought on things and perhaps keeping both ways of using the configs in place for now is prudent, just document both of them, and then later on down the road (once Solid is more prevalent) decide which system to keep at that time.

joshwilsonvu commented 3 months ago

Thanks for all your thought and effort in writing this up. Some context in why the package is structured this way:

Standalone

eslint-solid-standalone isn't meant to be used by ordinary projects; its main (and probably only) purpose is to power linting on https://playground.solidjs.com.

ESLint (at least v8) claims that importing only its Linter class is all you need to do to be able to use ESLint in a non-Node environment, but if you look at standalone/rollup.config.mjs, you'll see how much complexity it takes to strip out the parts of ESLint, typescript-eslint, and their deps that don't work outside Node. I gave ESLint v9 a shot but lots of different things broke and I felt it wasn't worth the effort.

Furthermore, all of those custom build-time transformations can break any time eslint-plugin-solid or those packages are updated. So it's most practical to keep it in this repo, rather than in https://github.com/solidjs/solid-playground.

Why not a monorepo then? It's chiefly because I want the README at the root to be the plugin's README, not just one for the workspace root, for a good experience visiting the repo. But if we can reasonably deal with that, converting to a monorepo would be great.

ESM / "exports" / CJS

Until v9, ESLint has been a stick in the mud about ESM support. And upgrading to v9 is a challenge. So for as long as it's relatively feasible, I see no reason not to support version 8 and below (I run tests down to v6).

I've written about why I want to support import config from 'eslint-plugin-solid/configs/recommended, not just plugins.configs['flat/...'], here.

It's a little unclear to me whether switching to use package.json "exports" instead of exposing "main" and implicitly exposing nested paths like /configs/recommended.js can worth with ESLint v8's CJS focus. I certainly can't expose ESM only and expect older setups to work. But I guess even with CJS, currently supported Node versions support "exports", right?

It sounds like tsup is the way to go for exposing both CJS and ESM. With multiple entry points, it could support /configs/*. Definitely open to using it for the plugin, maybe not standalone since that's more fragile.

Other

Appreciate any thoughts on this. Thanks again!

joshwilsonvu commented 2 months ago

Converted to a monorepo in #150 🎉