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

Adopt ES2015+ features in Unified collective #4

Merged
merged 1 commit into from
Aug 18, 2021

Conversation

ChristianMurphy
Copy link
Member

@ChristianMurphy ChristianMurphy commented Apr 5, 2020

@ChristianMurphy ChristianMurphy added 📚 area/docs This affects documentation 🗳 blocked/vote This cannot progress before voting is complete 🙉 open/needs-info This needs some more info 💬 type/discussion This is a request for comments 🐢 platform/node This affects Node 👩‍⚕ area/health This affects community 🏡 area/internal This affects the hidden internals labels Apr 5, 2020
@ChristianMurphy ChristianMurphy requested review from a team April 5, 2020 23:03
@ChristianMurphy ChristianMurphy changed the title Adopt ES2015 through ES2019 features in Unified collective Adopt ES2015+ features in Unified collective Apr 6, 2020
@ChristianMurphy
Copy link
Member Author

ChristianMurphy commented Apr 6, 2020

related but not required by this.
If unified wanted to specify some officially supported Node versions https://github.com/mysticatea/eslint-plugin-node could be used to verify the features used are fully supported by the officially supported version(s) of node (in addition to the test suite).

@wooorm
Copy link
Member

wooorm commented Apr 6, 2020

Controversial opinion™: ES6- is also ES6+. 😅

I think i’m in the minority with my personal opinions on these matters: class is sugar for prototypes; const is not a const; Promises are also callbacks; async/await results in waterfalls instead of parallel; arrow functions are shorter but you’re not using 1-letter variable names either: the word function is nice to have; function hoisting is great; while loops are the fastest and clearest there are.

All these new features are different, which is fine, but that doesn’t make them inherently better, nor worse. In some cases they are great! Async/await is really nice. Arrow functions are great for “lambdas”. Etcetera.

To summarize, I don’t have anything against ES2015+ features, but I have problems with “Adopt ES2015+”:

  • “Adopt ES2015+” typically means ban anything that existed before ES2015, often enforced by linters or formatters such as lebab. I think this is bad.
  • Using many different features leads to complexity (also applies to ES2015-: using === everywhere is less complex that using both == and ===)
  • ES2015+ features don’t remove the previous (confusing?) parts of ES, and are not necessarily easier: I’ve seen people struggle with this as they used arrow functions, thinking it was only better way to write functions, but then not being able to access the context object they expected in their event handlers.

These problems are fine in build scripts, it works well for a compiler like MDX, it’s a good decision if you’re building an app on top of unified. I think it works well in those cases because then it’s the end product: you’re in control. Often those things don’t last very long (a web app is rewritten from jquery -> backbone -> angular -> react -> X every Y years), in comparison to something like unist-util-is

TL;DR: Change is not good in and of itself.


So then the question is: what will this solve! What will this help? You point out:

Motivation: Easing contribution for new members

The State of JS survey 2019, indicates that 85-90% of JavaScript developers have used ES2015 features [1], which holds consistent with the 2018 results where 86.3% indicated they had used it and would use it again [2].

(I’m one of the people that chose those options too btw 😉)

But, what is the percentage of people that used var or function (rhetorical question)?

What you sent me in DMs is interesting!:

Simplifying code in Unified for new contributors (most devs I've talked to aren't as comfortable working with prototype as they are with class)

Prototypes aren’t great, true… I’m guessing that those people know class structures in other languages (java?). Are they confused by the differences between JS classes and other classes?
I have a feeling that ES2015- is instead simpler for new contributors, are there other reasons you see it otherwise?

Motivation: Allow for additional instrumentation and tooling

Tools like ESLint provide additional analysis for features using ES205-2019 syntax. [3] [4]

The rules in [3] (ESLint) catch new problems that are introduced by ES2015+ features, but the also don’t inherently make the current code better?
[4] (eslint-plugin-promise) seams like a great addition we could already add! (aside: maybe we should start making our own eslint/xo preset).

Drawback: Changes are Primarily Internal

These changes would not change the public API of Unified, VFile, Remark, etc. This would be a set of internal changes which could benefit maintainability long-term, but would not have an immediate benefit for downstream adopters.

Why do you think this improves maintainability?

Drawback: Legacy Support

While many browsers and all active release lines of Node support ES2015-ES2019. Not all platforms do, Nod.js 4 and earlier, Internet Explorer browser, and Rhino do not fully support ES2015. [5] To continue use on these platforms code would need to be transpiled to ES5, using Babel, TypeScript, or a similar tool.

We’re currently in a state where some our our (dev) dependencies are dropping support as well. This annoys me because it forces us to update too, and publish a major release, on that dependency’s schedule. Even if we’d set a unified policy to support Node X to Y, if our dependencies move faster, we can’t update, leading to potential security problems. Now I really don’t care about Rhino or IE10 or so, but there’s no harm in supporting a bit more that the current LTS either...


P.S. one thing I’m pretty excited about is ESM!.

@ChristianMurphy
Copy link
Member Author

ChristianMurphy commented Apr 6, 2020

Thanks for the feedback @wooorm! 🙇‍♂️

TL;DR: Change is not good in and of itself.

TL;DR: I agree.
There is a difference between change for change itself.
And change to better match the syntax with the intent.

I'm not saying all ES2015- is bad, or that all ES2015+ is good.
Some of the problems ES2015+ was updated to solve, are problems unified has, in these cases the new syntax may be a good fit.

Also note that this is not all or nothing, we can adopt parts of the recommendation from the RFC without needing to adopt all of them, and the parts adopted could be staggered over time if wanted.


I have a feeling that ES2015- is instead simpler for new contributors, are there other reasons you see it otherwise?

I'm looking at it from the perspective of "where are new contributors coming from?".
Several drivers for adoption include MDX, Gatsby, Prettier, and Storybook.

A common theme through these, they are primarily front end and they are primarily related to React.
Coming from a React background, developers will be familiar with class, let, const, and destructuring.
These patterns are used heavily in the documentation, examples, and tutorials.

It is important to note that these are not React specific, these are JavaScript language features.
React, Vue, Angular, and Web Components all leverage and encourage features added in and after ES2015.

And while JavaScript doesn't remove features, there is a tendency to use a subset of the language "the good parts" which evolves over time.
Developers (especially new ones) coming from a front-end background tend more familiar with the syntax which has been primarily used by front-end frameworks for the past 3-5 years.

I’m guessing that those people know class structures in other languages (java?). Are they confused by the differences between JS classes and other classes?

I haven't seen this as much.
The trend I've seen is front-end devs who are JavaScript first.

“Adopt ES2015+” typically means ban anything that existed before ES2015, often enforced by linters or formatters such as lebab. I think this is bad.

To offer a different perspective.
New syntax features and syntax sugar reflect architectural trends in the language.
These trends often stem from syntax causing confusion (E.G. var), nor not being well suited for an abstraction they are trying to represent (E.G. prototpevsclass`).
It isn't necessarily saying "ES2015- is bad", nor does it ban all of ES2015-. it focuses on specific features saying "these syntax features were often cause confusion or bugs, here is an alternative".

Why do you think this improves maintainability?
We’re currently in a state where some our our (dev) dependencies are dropping support as well

I see these two as somewhat going together.
frameworks and libraries across the JavaScript ecosystem, front-end and back-end are moving towards adopting new language features.
Adopting syntax features that fit with unified's intended API, like using class for vfile can make the codebase more approachable, which brings more contributors.
I consider having more developers being able and willing to contribute to be one signal of maintainability.

Even if we’d set a unified policy to support Node X to Y, if our dependencies move faster, we can’t update, leading to potential security problems.

How is that different than from today?
A breaking change is a breaking change.

If it's in a dev dependency, it isn't a breaking change for our adopters, and way may need to remove some checks on CI (annoying), but no impact downstream.
If it's a breaking change in a (non-dev) dependency, calling it a non-breaking change because we don't have a node versioning policy, doesn't make it less of a breaking change for developers using the unsupported version of Node. 😅

there’s no harm in supporting a bit more that the current LTS either

A different perspective.
Unified (and libraries in general) depend on their platform (Node.js) to maintain platform level APIs.
If and when adopters run into issues caused by a platform API (on a version no longer supported), library maintainers get caught between a rock and a hard place, the fix can't be made upstream, and trying to fix the issue at a library often ranges between an ugly hack to impossible.
Open source maintainers have limited time and resources, it may be better to drop support proactively, than to try to support unsupported platforms. (Note this may just be documentation changes, not code changes)

@Murderlon
Copy link
Member

Seems like almost all arguments have been brought up already. Just adding my two cents.

  • I think it’s become clear that this isn’t a binary switch and everything should be done in ES2015+ or not. That’s good. Something I particularly liked was “change to better match the syntax with the intent” — @ChristianMurphy.
  • My focus of this RFC lies more in “adopt” than change (retroactively). Meaning I’m mostly interested in agreeing on using (certain) ES2015+ features from now on and less in going through the entire ecosystem bottom-up to change it.
    • On that note, does it make sense to hold that off to combine with another syntactic change like ESM at the right time? I remember a discussion somewhere about adopting it, in which @wooorm felt it was too early (I think), but I can’t find it.

@ChristianMurphy
Copy link
Member Author

[About ESM[ I remember a discussion somewhere about adopting it, in which @wooorm felt it was too early (I think), but I can’t find it.

unifiedjs/unified#76

one thing I’m pretty excited about is ESM!.

On that note, does it make sense to hold that off to combine with another syntactic change like ESM at the right time?

Would there be interest in another RFC to discuss ESM?
Or adding it as a part of this RFC?

@wooorm
Copy link
Member

wooorm commented Apr 7, 2020

To me, this discussion when only posed (due to my comments) as “ES2015+ or ES2015-” leans towards yak-shaving (as in, lots of discussion about not very meaningful changes). Though we all seems to agree that we don’t want that to be the topic. We do seem to agree that some ES2015+ features, notably ESM, would be good (albeit uncertain about how and when, also due to my comments). I believe that ESM is a fruitful conversation to have because it’s more tangible: at least to me, I can scope what the changes would mean to the 300+ projects. Deciding on ESM (and when) would lead to deciding on minimum runtime versions, then opening the doors towards refactoring into things such as classes, and thus practically implementing/solving this RFC.

Does that match how you all see this conversation?

@ChristianMurphy
Copy link
Member Author

ChristianMurphy commented Apr 7, 2020

I can scope what the changes would mean to the 300+ projects.

To be clear, this PR is labelled as "collective" because it would touch projects in several orgs.
It wouldn't necessarily be all 300+ repositories.
The repos that would benefit the most would be vfile-message, vfile, unified, remark-parse, remark-stringify, rehype-parse, and rehype-stringify.

I can scope what the changes would mean to the 300+ projects. Deciding on ESM (and when) would lead to deciding on minimum runtime versions, then opening the doors towards refactoring into things such as classes, and thus practically implementing/solving this RFC.

I agree that ESM could be a lead in for refactors across the ecosystem.
The reason I'm cautious is because ESM breaks more versions of Node than the features listed currently in this PR.

Native ESM with no CJS fallback may also require some testing with bundlers for browser usage, we'd need to do some research into how widely "type": "module" in package.json is supported.
Particularly with Webpack which is used for most MDX builds and all Gatsby builds, also Parcel and Rollup which are alternatives supported by MDX.

It may be worth it for ESM to have it's own RFC to discuss.

@Murderlon
Copy link
Member

I feel with ES2015+ features, ESM, and potentially officially supported Node versions the scope of this RFC will become too big. Unfortunately they aren’t mutually exclusive either. I think it makes sense to discuss ESM in a different RFC, as @ChristianMurphy mentioned, it requires research on its own. Choosing whether to adopt ES2015+ features in general shouldn’t stand in the way of the ESM RFC, we can always decide later if applying the decisions from the RFCs can be combined.

Back to the intention of this PR. I mentioned before that I was more interested in allowing ES2015+ features instead of changing it throughout the ecosystem. But after thinking about it a bit more, I think it should be more about the latter. As choosing what makes sense for new projects is only natural and perhaps doesn’t need an RFC. Look at micromark, for instance, with its classes, generators, and TypeScript.

So perhaps to make this RFC pass (or not) the focus should be on what, where, and why ES2015+ features would be an improvement.

The repos that would benefit the most would be vfile-message, vfile, unified, remark-parse, remark-stringify, rehype-parse, and rehype-stringify.

@ChristianMurphy could you elaborate on what ES2015+ features these projects would benefit from and why?

@ChristianMurphy
Copy link
Member Author

could you elaborate on what ES2015+ features these projects would benefit from and why?

Most of these repos are, or include a constructor pattern.
Often using prototype to generate a class-like structure.
class does the same thing, in a way that structurally shows that the pieces are part of a single construct.

The other feature that could help a lot would be const, if #5 moves forward, const ensures a type stays consistent, which matches with how TypeScript models data. var it a lot more dynamic, in vfile/vfile-message#8 several usages of var where the variable was reused for several different meanings and with several different types, tripped up TypeScript.

@Murderlon
Copy link
Member

Definitely agree with the class example!

The other feature that could help a lot would be const, if #5 moves forward, const ensures a type stays consistent, which matches with how TypeScript models data. var it a lot more dynamic, in vfile/vfile-message#8 several usages of var where the variable was reused for several different meanings and with several different types, tripped up TypeScript.

Correct me if I’m wrong but this sounds like an implementation problem rather than var inherently tripping up TS in some cases. Nonetheless, I agree const/let would also help in communicating intend.

To conclude, my personal preference would be in accepting this RFC for the bigger projects, like the examples @ChristianMurphy gave, with some ES2015+ that benefit matching syntax with intend, or help author better types. I don’t see a lot of benefit in also changing the utilities or other areas (for now).

@ChristianMurphy
Copy link
Member Author

ChristianMurphy commented May 18, 2020

Correct me if I’m wrong but this sounds like an implementation problem rather than var inherently tripping up TS in some cases

No even an implementation problem per se.
It works as is.
More that static type checkers don't handle dynamic changing of the type on a variable in the same scope, which var enables.

@wooorm
Copy link
Member

wooorm commented May 19, 2020

For the classes/prototype: I’d prefer to move away from them entirely, where possible 😅
They make sense when inheriting, such as from Error for VMessage, and do agree we should go with class there.
They make sense for VFiles too.
But I hope that micromark can use a more functional approach.

More that static type checkers don't handle dynamic changing of the type on a variable in the same scope, which var enables.

let will also enable that, no?
If that happens, it can either be intentional or unintentional. When the latter, whether it’s var or let, that should be fixed! I personally really like dynamic languages, so by extension, I like that JS allows that!


I’m up for doing this (depending on the ES feature)! Indeed, as @Murderlon points out, for the projects that fluctuate more that the lowest-level utilities; to do it for the projects that benefit from it. But I would like to do it together with a supported environments policy. It’s a breaking change. And I would prefer to combine it with other breaking changes.

@wooorm wooorm changed the base branch from master to main June 21, 2020 13:24
@wooorm
Copy link
Member

wooorm commented Jun 25, 2020

To kickstart this again:

I am, perhaps more so now than in the previous comments, appalled by the state of the JavaScript ecosystem, which I feel originates in part because of an unhealthy appreciated for new syntactic sugar and the latest and greatest build tools. While these features or these tools are not bad on their own per se, the JavaScript ecosystem as a whole is in a bad state. I think we (@ChristianMurphy, @Murderlon, I) can agree that we have all seen many issues stemming from these tools and features.

I would want to restart this problem not from a perspective of what features can we use, but rather: how can we be a part of a faster, safer, stabler, more maintainable, and also future proof ecosystem.

What does that mean? What will ESM bring? How do we provide both cjs and mjs? How do we support Node and Browsers. Can we make sure that folks who compile their code, get the latest features, whereas folks who don’t compile, get stuff that works in most browsers?

@ChristianMurphy
Copy link
Member Author

ChristianMurphy commented Jun 25, 2020

appalled by the state of the JavaScript ecosystem [...] I feel originates in part because of an unhealthy appreciated for new syntactic sugar

How so? You've expressed concerns around module types in the past, but not specifically with syntax itself.

latest and greatest build tools

All the features in the RFC are natively supported in Node and in most browsers.

What will ESM bring?
JavaScript ecosystem as a whole is in a bad state

@wooorm I think you're starting a new discussion, unrelated to this RFC, this RFC intentionally does not include ESM, as the different module types JavaScript supports are a complex and multifaceted topic.

@wooorm
Copy link
Member

wooorm commented Jun 26, 2020

I think you're starting a new discussion

That is mostly correct: because while I agree that the points in Motivation are worthwhile, I don’t feel strongly that Detailed design solves them, whereas the Drawbacks are real.

Specifically, for the detailed design, I believe

  • 1 (class) — to be indeed better
  • 2 (default params), 7 (async/await) — to have downsides (no null, waterfall instead of parallel)
  • 3 (let, const) — to be basically syntax sugar. There’s something to say about intent, but it also opens up a discussion between going with let only? const always except when needed? Etc.
  • 4 (arrow functions), 5 (rest) — to be syntactic sugar without any real benefits.
  • 6 (Promise) — to be very useful and I believe already supported in most places!

(All these features are nice on their own, especially in apps/scripts, etc, the questions is though: is it worth rewriting things, and if so: how exactly)

I am fine with these changes, but two more downsides:

  • It leads to a lot of talk about how to exactly implement it, styleguides, presets, etc.
  • It takes time to rewrite things

I think to reach the points in Motivation, we need more, such as:

  • Adopt ESM + CJS bundles everywhere
  • Guarantee that certain Node/browser versions are supported
  • ESLint preset

These could be different RFCs, but I feel those ideas touch on this RFC, and they give this RFC more power.

@ChristianMurphy
Copy link
Member Author

On the topic of ESM, https://github.com/mikeal/limbo may be a good option for adding support in a non-breaking way.

@wooorm
Copy link
Member

wooorm commented Aug 24, 2020

@TrySound you mentioned on Twitter that you might have ideas on how to do ESM for micromark? Pinging you here to discuss more of that, if you’re interested!

@TrySound
Copy link

Converting to esm is easy. I did it locally with a few replace-in-files commands for micromark. Distribution will include main, module for bundlers and exports fields for node. Default export in entry points should be replaced with named one to prevent issues with ts definitions.

@wooorm
Copy link
Member

wooorm commented Aug 25, 2020

Nice!

Are you saying that there is no way to use default exports at all? We currently use TS definitions in several projects with CJS “defaults exports”. What is TypeScript’s problem with default exports?

While micromark is new and it’s expected that stuff’ll break now, we are also approaching a point where the hundreds of projects in the unified collective move to ESM. It would be preferable if we can do that without breaking anything.

@TrySound
Copy link

This describes the issue
alexreardon/memoize-one#37

@wooorm
Copy link
Member

wooorm commented Aug 25, 2020

Interesting read, thanks. In short, that seems to indicate that both a default, and a named export, are used. In .cjs:

Object.defineProperty(exports, '__esModule', { value: true });
// ...
exports.default = memoizeOne;
exports.memoizeOne = memoizeOne;

And .mjs:

export default memoizeOne;
export { memoizeOne };

And .d.ts:

export default memoizeOne;
export { memoizeOne };

@TrySound
Copy link

Now yes, but it's mostly migration path. Ideally to use only named exports.

@ChristianMurphy
Copy link
Member Author

One path forward with ESM is being discussed at micromark/micromark#27

wooorm pushed a commit to micromark/micromark that referenced this pull request Oct 12, 2020
Closes GH-27.
Related to unifiedjs/rfcs#4.

Reviewed-by: Christian Murphy <[email protected]>
Reviewed-by: Titus Wormer <[email protected]>
wooorm pushed a commit to micromark/micromark that referenced this pull request Nov 20, 2020
* Add tests for local babel plugins
* Replace babel w/ rollup
* Remove unused constant

Related-to unifiedjs/rfcs#4.
Related-to GH-27.
Related-to GH-28.
Closes GH-35.

Reviewed-by: Titus Wormer <[email protected]>
@ChristianMurphy
Copy link
Member Author

ESM and a limited set of ES6 features rolled out to most of unified as part of unifiedjs/unified#121
Would it make sense to merge this? or close it?

@wooorm wooorm merged commit 6b7762f into unifiedjs:main Aug 18, 2021
@wooorm wooorm added 💪 phase/solved Post is done and removed 🗳 blocked/vote This cannot progress before voting is complete 🙉 open/needs-info This needs some more info labels Aug 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📚 area/docs This affects documentation 👩‍⚕ area/health This affects community 🏡 area/internal This affects the hidden internals 💪 phase/solved Post is done 🐢 platform/node This affects Node 💬 type/discussion This is a request for comments
Development

Successfully merging this pull request may close these issues.

4 participants