rehypejs / rehype-minify

plugins to minify HTML
https://unifiedjs.com
MIT License
89 stars 16 forks source link

Publish separate package with `rehype-minify-enumerated-attribute` schema? #37

Closed karlhorky closed 3 years ago

karlhorky commented 3 years ago

Subject of the feature

The rehype-minify-enumerated-attribute schema looks super useful for things like minifiers and linters.

Problem

I have recently done some research for a potential lint rule in eslint-plugin-react, and this schema looks like it would help a lot with the issue of "which attributes are already set to their default values?"

Expected behavior

Separate package, similar to html-element-attributes

Alternatives

Use another library, such as one from the list below:

wooorm commented 3 years ago

could you confirm whether the file currently matches what you’d expect? https://github.com/rehypejs/rehype-minify/blob/main/packages/rehype-minify-enumerated-attribute/schema.js

Then I can publish it here as html-enumerated-attributes or so, similar to html-url-attributes

karlhorky commented 3 years ago

It's looking mostly good, just opened some new issues with feedback about inconsistencies here: https://github.com/rehypejs/rehype-minify/issues/38 https://github.com/rehypejs/rehype-minify/issues/39

Also, I would mirror ljharb's comment that the casing should be the HTML version of the attributes (instead of hast), if possible:

https://github.com/yannickcr/eslint-plugin-react/issues/2866#issuecomment-740918159

ljharb commented 3 years ago

What I'd hope for is a common core package, that's used by one package for react jsx, and by another package for HAST.

That way, changes that are jsx-only needn't cause churn here, and changes that are HAST-only needn't cause churn for eslint-plugin-react, but changes in the common core necessarily cause churn in both - and dependencies can be more granularly specified (so that fewer unnecessary things are installed).

wooorm commented 3 years ago

Hi folks, thanks for all the activity, ✨ sorry for being silent: I’ve just been taking this time to try and focus on some other OSS projects!

I think we’re all pretty close to each other. I totes understand your last comment, Jordan! But still: I do think the simplest way to go is hast casing for the core package. Or React casing. Both are fine. The reason is that they’re almost identical: a very short list of exceptions.

The HTML -> React or HTML -> hast mapping is rather long. Lots of unneeded complexity for the both of us. It may be nice for some other folks, but I’m doubting there’s that many folks interested in it!

And: I don’t want to block this. I’ll get back to it soon, but for now: the schema is published already but as a file in a small package. And if any one wants to publish it (as React / HTML) casing, or copy-paste it into the code, that’s also fine with me!

karlhorky commented 3 years ago

Hey @wooorm , would you still be open to publishing the html-enumerated-attributes package still (either with hast or React casing)?

wooorm commented 3 years ago

added it, using HTML casing!

ljharb commented 3 years ago

Awesome! I don't see an "engines" field; can you confirm how far back it will work? eslint-plugin-react supports node 4+.

wooorm commented 3 years ago

ESM only 😬

ljharb commented 3 years ago

@wooorm oof, that means it's incompatible with virtually everything and won't work at all for us. Any chance you could also provide a CJS implementation?

wooorm commented 3 years ago

Yeah, it’s a pickle. I’m afraid I’m rather hard headed on this one, I ported all my packages over to ESM, and am not providing dual (in my experience dual doesn’t work well in existing bundlers). This is less of an issue for my other projects because they have a previous CJS version.

Could you use an import() expression perhaps, assuming that your users are Node users? While not a Node 4 thing, as I believe you are aware, import() does work in CJS (albeit that I think ESLint doesn’t support async rules?)

On the “that means it's incompatible with virtually everything” side, I disagree. 1% of all npm packages are ESM already! https://twitter.com/wooorm/status/1431289978961764357. A small number of course, but I think it signals that it’s going relatively fast.

ljharb commented 3 years ago

No, dynamic import doesn’t go anywhere near that far back - and eslint works async, so no part of the eslint ecosystem can ever use dynamic import, or native ESM - and the same applies to the babel ecosystem.

ljharb commented 3 years ago

You can of course do what you like, but it defeats the entire purpose of making separate packages if they can’t be maximally shared, and “esm only” guarantees they can’t be.

So far what I've seen is that authors who move a package to pure ESM end up forcing their consumers to migrate off of it to a more compatible solution.

wooorm commented 3 years ago

While I completely understand that me making this ESM technically solves this issue but doesn’t solve the practical issue of making it work in eslint-plugin-react, I want to point that I’ve been fine with someone else publishing it, however they please, and me using it here, and want to reiterate that I still am!


So far what I've seen is that authors who move a package to pure ESM end up forcing their consumers to migrate off of it to a more compatible solution.

This is turning into an ESM conversation, which I’m fine having and sharing my views on, but I also assume that you have good and thoughtful views on this. So if you don’t want to have this, that’s also fine!

I understand that (gut) reaction but I’d add that in my experience:

a) there are a few very vocal voices, I totally understand that it sucks that things change, but in my experience most people get over this phase, and alternatively the previous version is usable b) there are also people jumping from Jest to Ava, Gatsby to Next, and such, because the first doesn’t support ESM and the latter does c) similarly to the b), there are also more silent newcomers who choose ESM over CJS because the latter looks “old”, and the former uses modern syntax. I have a bit more empathy for newcomers getting into JS having a hard time with their from StackOverflow copied import not working than experienced folks who perfectly know what’s going on but just don’t like change. (of course we’re not there yet, but I want to do my bit in moving the ecosystem forward) d) if someone wants to use something else — that’s fine with me!

ljharb commented 3 years ago

If I published the package as CJS, would you depend on it? The only benefit here is if we're both sharing the same data source.

If not, then, if it were published as a dual package, would you depend on it? (it's simplest to keep it pure CJS, since ESM can import CJS)

wooorm commented 3 years ago

Both, yes.

But for me to do it, what’s the time frame for you/eslint-plugin-react dropping CJS? As Node 4 is still supported, which was released 6 years ago and has been EOL for ages — I don’t want to maintain a CJS or dual package in 2027.

ljharb commented 3 years ago

In general, for all of my hundreds of packages, I don't ever drop support for anything unless there's a very strong reason - most of my packages still support node 0.4.

Obviously if in 2027, nobody actually uses anything but ESM, then there'd be no benefit to supporting anything prior to that, but that's dictated by the community, and shouldn't be artificially forced by authors.

I'm happy to maintain it, supporting node 4 (likely, 0.4, since it takes almost no effort to support everything), for as long as I'm alive - as long as you'd depend on it.

I'll try to get something published in the next week.

wooorm commented 3 years ago

there's a very strong reason

In my opinion ESM is one. Note that you can keep on maintaining previous versions, especially for security fixes.

but that's dictated by the community, and shouldn't be artificially forced by authors.

I’d argue that you and I are part of the community and that there’s nothing artificial about using standard JavaScript features supported in all maintained JavaScript engines. The artificial part in my opinion is using Node-specific features that don’t work in most of those engines.


I'm happy to maintain it […] as long as you'd depend on it.

I'll try to get something published in the next week.

Awesome!

wooorm commented 3 years ago

I’ve published v0.1.0 and have added both of you as admins on npm: https://www.npmjs.com/package/html-enumerated-attributes!

ljharb commented 3 years ago

In my opinion ESM is one. Note that you can keep on maintaining previous versions, especially for security fixes.

Adding "exports" is a breaking change, and this is a good reason to do a bump - but not a good reason to drop node versions, since dual packages do in fact work fine (if it breaks on a bundler, then don't use a broken bundler), and can be added semver-minor once "exports" exists.

ChristianMurphy commented 3 years ago

if it breaks on a bundler, then don't use a broken bundler

I appreciate the sentiment, and agree that it would be nice if build tools could smooth over more of this, create-react-app/webpack-4 does not https://github.com/remarkjs/react-markdown/issues/518. Right now CRA makes up significant segment of the unified community, pure ESM works seamlessly with CRA, dual packaging is horribly broken.

And even if dual packaging did work well, it adds another burden on library maintainers.

not a good reason to drop node versions

I agree, but likely for very different reasons than you have. Community libraries should not have to maintain support for end-of-life node, period. If the node foundation and it's backers don't have the resources to support a node release, it shouldn't fall on community library maintainers deal with any issues left behind in the end of life platforms in perpetuity. That alone is reason enough to drop official support for a node version.

That said, I don't think libraries should go out of the way to break backwards compatibility either. For unified it feels that ESM makes sense, it works natively on Node LTS, it greatly reduces the tools needed for use on web, deno, and other platforms, and can still support older versions of node through transpilers/bundlers/shims.