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

Remove harmful stage 1 polyfill recommendation #33

Open
wants to merge 3 commits into
base: gh-pages
Choose a base branch
from

Conversation

ljharb
Copy link
Member

@ljharb ljharb commented Dec 17, 2021

It seems that this document is inadvertently recommending that folks publish polyfills in stage 1, which is incredibly dangerous for web compat, rather than in stage 3, which is the appropriate time.

This PR does not change any stage advancement criteria; in spec lingo, it is editorial, not normative.

It seems that this document is inadvertently recommending that folks publish polyfills in stage 1, which is incredibly dangerous for web compat, rather than in stage 3, which is the appropriate time.

This PR does not change any stage advancement criteria; in spec lingo, it is editorial, not normative.
@bakkot
Copy link

bakkot commented Dec 17, 2021

Stage 1, or 2 at the latest, is an appropriate time to experiment with implementations. They just shouldn't be shipped.

Temporal has had a polyfill since long before it got to stage 3, and was very useful, for example.

@ljharb
Copy link
Member Author

ljharb commented Dec 17, 2021

Totally; which is why i changed it to "reference implementations" - essentially what I want to capture (and am open to suggestions) is that nobody should be shipping, or publishing, any code that modifies the environment until stage 3.

The word "polyfill" is synonymous for many with "published code that automatically modifies the environment", and thus that word shouldn't be used imo in a stage prior to 3.

@js-choi
Copy link

js-choi commented Dec 17, 2021

Since the issue is deployment to production, perhaps “polyfill” in this pull request should be clarified to “experimental polyfill” (even if this is redundant, maximal clarity can be good).

The note could also be made beefier to explicitly warn: no proposal’s polyfill should ever be used in production code before it reaches Stage 3.

@zloirock
Copy link

zloirock commented Dec 17, 2021

Sorry, but it's a harmful change. I agree with @bakkot and @js-choi.

@ljharb
Copy link
Member Author

ljharb commented Dec 17, 2021

@js-choi i'd be content with that if we could include "published to npm" or similar as well.

@zloirock
Copy link

published to npm

🤦

@zloirock
Copy link

zloirock commented Dec 17, 2021

Just interesting, how to experiment with a several early-stage proposals, how they work together, when you can't load polyfills of them from NPM or something like that -)

@ljharb
Copy link
Member Author

ljharb commented Dec 17, 2021

Existing proposals haven't had any difficulty doing so since this process's inception - if that becomes an obstacle in the future, that'd be a great thing to learn.

@zloirock
Copy link

zloirock commented Dec 17, 2021

@ljharb you are deeply mistaken. The most of JS standard library features had feedback and changes on the early stages after experiments with polyfills.

@ljharb
Copy link
Member Author

ljharb commented Dec 17, 2021

That has not been my experience in the past 7 years; the useful early stage feedback has come from userland implementations that do not modify the global.

@zloirock
Copy link

If you think so, that meant that you do not monitor the proposals for the past 7 years. For example, the latest presented @js-choi proposal - and you could see the same in most of the standard library proposals, and it's only from me - and from implementations, that modify the global.

@acutmore
Copy link

One potential advantage of “reference implementation” over “polyfill” is that it would include syntax based proposals too.

@ljharb
Copy link
Member Author

ljharb commented Dec 17, 2021

@acutmore it also describes the polyfill.js file that appears in many older proposals, but is responsibly never published anywhere.

@ljharb
Copy link
Member Author

ljharb commented Dec 17, 2021

@zloirock and all of that feedback is helpful for stage 3, and won't come up in plenary until stage 3 advancement reviews, or stage 3 implementation feedback. So, thanks - and your feedback was helpful - but none of that feedback would be "too late" if it were provided during stage 3.

@zloirock
Copy link

@ljharb without this feedback is impossible to write a proper spec text that's required for stage 2. Without this feedback is impossible to find conflicts with other proposals, that's required for the design of this proposal on the early stages. About what stage 3 do you say?

@ljharb
Copy link
Member Author

ljharb commented Dec 17, 2021

It's not required for stage 2 - which only requires initial spec text - it's required for stage 3, and spec reviewers handle that. If it could only be discovered by implementations, then that can safely come in during stage 3, as is happening now for current stage 3 proposals.

If you find value in implementing it sooner, great! None of that feedback comes from publishing it, only implementing it. I often author polyfills in early stages, with tests, for this purpose! However, I don't publish them - or even make them public on github - until the proper time.

@zloirock
Copy link

Stage 3 is an almost complete feature and on this stage detected mainly web compatibility problems. Stage 3 is a flag that something should be implemented in JS engines, the changes after stage 3 will be breaking for engines and will cause more significant problems than early-stage polyfills.

@zloirock
Copy link

None of that feedback comes from publishing it, only implementing it.

The feedback comes from experiments with published polyfills. You could find it in many proposals repos.

@ljharb
Copy link
Member Author

ljharb commented Dec 17, 2021

That has not been the case for Temporal (which has had extensive stage 3 changes), and was not the case for every other proposal I can recall, all of which we have both authored polyfills for.

@zloirock
Copy link

Maybe because I still didn't make a polyfill for Temporal? -) If you can't remember other cases - it's just a problem with your memory or attentiveness.

@ljharb
Copy link
Member Author

ljharb commented Dec 17, 2021

Temporal had a polyfill, the entire time, and it failed to catch most of these issues - the problem there is that it's a large spec. Most proposals are small, and thus most of these kinds of issues are caught purely on review.

@zloirock
Copy link

Temporal had a polyfill

I did not say otherwise.

failed to catch most of these issues ... most of these kinds of issues are caught purely on review

But not all.

@zloirock
Copy link

zloirock commented Dec 17, 2021

However, why do we talk only about polyfills? Let's say the same about transpilers plugin and will propose Babel and TypeScript unpublish, for example, decorators or pipeline operator plugins - it's the same implementations as polyfills.

@ljharb
Copy link
Member Author

ljharb commented Dec 18, 2021

Because there's no risk to web compatibility from that, because transpilation is done prepublish. It's unique to API proposals - transpilers are only for syntax proposals.

@zloirock
Copy link

because transpilation is done prepublish

Nope, not always. Now it's usual practice to transpile dependencies and publish not transpiled version of code.

@ljharb
Copy link
Member Author

ljharb commented Dec 18, 2021

In specific ecosystems, like RN and ember, that is true - but it is nowhere near common, usual, or advisable, and either way, that code would never land on the web untranspiled, so the risk isn't there.

@zloirock
Copy link

zloirock commented Dec 18, 2021

So why do I commonly see such problems on NPM, why do I see that someone trying to transpile core-js and have circular dependencies because of this, why do I see published TS with decorators and many other cases?

@ljharb
Copy link
Member Author

ljharb commented Dec 18, 2021

I'm sure there are a number of cases where people do the wrong thing. That doesn't cause a web compat issue, because they can't deploy that code to the web, because it'd be a syntax error. The web compat issue is when you can deploy something that works, and the real proposal changes the functionality.

@js-choi
Copy link

js-choi commented Dec 18, 2021

I see; thanks for the .match metaprogramming example.
Since we’re talking about experimentation that does not involve any production code…is there a particular reason why defining a separate metaMatch function—one which would use an also-exported matchSymbol etc.—would not be adequate for experimentation? We’re not talking about adapting existing production codebases here, after all; we’re talking about demos and experiments. (Even if we are adapting an old codebase for experimentation purposes, transforming code that uses .match to use metaMatch should still work.)

@zloirock
Copy link

zloirock commented Dec 18, 2021

@ljharb ok, .matchAll 😂

@ljharb
Copy link
Member Author

ljharb commented Dec 18, 2021

It's pretty trivial to do it for experimentation or non-shimmed purposes - you'd just have to have fallback behaviors for when the symbol was absent.

@zloirock
Copy link

zloirock commented Dec 18, 2021

@ljharb and it will not be the implementation of the real future behavior since it will not be absent in the future engines.

@ljharb
Copy link
Member Author

ljharb commented Dec 18, 2021

That doesn't matter for an engine where it's not natively implemented. A perfectly compliant implementation is virtually never needed to provide useful feedback/functionality, especially for obscure stuff that basically nobody uses like RegExp subclassing.

@@ -80,7 +80,7 @@ <h2>Stages</h2>
</td>
<td>None
<td>Major
<td>Polyfills / demos
<td>Demos and experimental reference implementations (implementations that modify global built-in objects should not be published as libraries or used in production-deployed code; implementations that do not modify global built-in objects may be published)
Copy link

@js-choi js-choi Dec 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving this to a new conversation: I also think the “Experimental” in Stage 2 should be further clarified to:

Experimental engine implementations and other reference implementations (implementations still should not conditionally modify global objects using feature detection)

I think redundancy for the sake of clarity is desirable in this case. The word “Experimental” alone in this column for Stage 2 no longer makes sense, since Stage 1 now also uses the word “experimental” in the same column.

@zloirock
Copy link

@js-choi core-js have both - global and pure modes. In the pure mode, core-js does something like that, but it's not always possible. How do you think, who added transpiling of instance methods to Babel in the mode without global namespace pollution and such special cases, like you proposed? However, it's very limited for the case of experiments, global polyfills are much better for it.

@zloirock
Copy link

@ljharb Babel implements new JS features with usage RegExp subclassing, so it's a pretty loud statement that no one uses it - at least too many Babel users use it.

@ljharb
Copy link
Member Author

ljharb commented Dec 18, 2021

Babel's choice to be spec-compliant does not indicate that even a single person uses it. I have yet to see evidence that literally anything besides core-js's Symbol.match feature detection relies on the feature on the web, but I'd be happy to learn about such instances.

@zloirock
Copy link

zloirock commented Dec 18, 2021

Nothing to say 😂😂😂 So you think that no one use, for example, NCG with Babel?

@js-choi
Copy link

js-choi commented Dec 18, 2021

core-js have both - global and pure modes. In the pure mode, core-js does something like that, but it's not always possible. How do you think, who added transpiling of instance methods to Babel in the mode without global namespace pollution and such special cases, like you proposed?

Yes, the pure mode is great, and it should be encouraged. In addition, thank you for your work on Babel.

However, it's very limited for the case of experiments, global polyfills are much better for it.

I’m still not following this statement, sorry. I’m not sure why it would be that much better to have global prototypes for mere experimentation.

For example, if I wanted to experiment with a metaprogrammable .match, then I would be content with playing with metaMatch and matchSymbol imports in a throwaway playground. Yes, using imports is somewhat less ergonomic than directly using global properties, but that seems fine to me for playground experimentation. It’s not like I should be maintaining any code that relies on the feature—until the feature stabilizes at Stage 3, anyway.

For now, I have to disengage from this conversation to do other work, so I won’t be able to reply for a while; my apologies.

Copy link
Member

@leobalter leobalter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the way this is going. The language used in this PR indicates recommendation (should/may etc) and won't block anyone from shipping code they desire. It's important TC39 does not require in production publication of things we - by written process - intend to discuss and shape.

@zloirock
Copy link

zloirock commented Dec 18, 2021

@js-choi the problem is that helpers like metaMatch are not always applicable and the experimentation is not only some lines of your code - you could have some dependencies.

The simple and old example, from early 2015 - sometimes, Number constructor should be polyfilled for accepting octal and binary literals. But it's not polyfilled in the core-js-pure / bebel-runtime. Why?

In this case, Number constructor requires a wrapper. But some libraries have a such ugly check:

function isNumber(it) {
  return it.constructor === Number;
}

isNumber(Number(something)); // => false

In this case, it.constructor is a global Number, Number - non-global injected polyfill. Moreover, it was code from a testing library.

Sure, we could wrap each property access for preventing such problems - but anyway it will not prevent all possible problems. And it will make work on polyfilling / transpiling of this almost impossibly hard.

@ljharb
Copy link
Member Author

ljharb commented Dec 18, 2021

You're not describing experimentation problems, you're describing production problems.

@zloirock
Copy link

zloirock commented Dec 18, 2021

The testing of a new language feature is an experimentation problem.

@zloirock
Copy link

Just to be clear, this PR and position of some TC39 members nullify 8 years and many thousands if not dozen of thousands of hours of my work on improving the future JavaScript. If it will be accepted, this will not end with anything good for the language. I've finished.

@leobalter
Copy link
Member

All due respect, this hasn't anything to do with your work but a proper standardization process.

We can always standardize things that already exist in the multitude of implementation environments, as they already exist, but we should always thrive for finding the best solution for specific goals.

Experimentation, by definition, should not be something that sets a single option into stone. Whatever TC39 says here with these PR changes or not, won't change when implementations and people must have something in production. This is actually a pretty mild expectation and reflects the perception I have as a delegate. A polyfill being available was never something TC39 required to see in production before stage 3.

If a project decides to release things earlier, that's ok, knowing the lack of stability might require severe changes along the way. What it must not do is take that state and block other paths to be taken.

We've seen so many big changes in stage 2, and we've seen some broad availability limiting alternatives for discussions before maturity levels.

This is not any personal attack or aiming at any project but a slight wording improvement towards a solid standard process.

@zloirock
Copy link

@leobalter all this work significantly helped and helps to the standardization process and it's very close to it or you will argue otherwise? Early-stage proposals polyfills used only for experimentation, people know it, if someone uses it in production - it's his fault. Polyfill something is not set a single option into stone - polyfills can be changed. Many features that were polyfilled were significantly changed during the process and became the stable JS - but how many of them were broken because of polyfills? It's a very rare case.

@leobalter
Copy link
Member

I need more time to understand how this change is affecting you and perhaps a broader community involved in the process.

@js-choi
Copy link

js-choi commented Dec 18, 2021

From what I can gather in #33 (comment) and #33 (comment), there are some points of agreement:

  1. In experimental code, developers should experiment with TC39 API proposals in Stages 0–2. This is important.
  2. “Experimental code” can include short snippets, playgrounds, and large codebases – but none should be relied upon in production.
  3. In production code, developers should not mutate built-in global objects (conditionally using feature detection) with TC39 API proposals in Stages 0–2. This is also important.
  4. Developers can adequately experiment with most TC39 API proposals (like Array.fromAsync and Map.prototype.emplace) without conditionally mutating built-in global objects using feature detection. Instead, they can import helper functions (“pure mode” in core-js).
  5. There are a few exceptional TC39 API proposals (like Symbol.match and the binary/octal Number constructor) that would be more difficult for developers to experiment with, unless they mutate built-in global objects. This only applies to experimentation in large codebases with many dependencies – although it does not apply to experimentation in short snippets or playgrounds.

Hopefully we can all agree on these points, since they seemingly have already been affirmed by everyone.


With that said, even if early experimentation is important, the big disagreement seems to apply to only a small subset of experimentation. The affected experimentation has to fulfill the following conditions:

  • The experimentation has to involve one of only a few exceptional API proposals (like the Number constructor or Symbol.match). Most API proposals (like Array.fromAsync or Map.prototype.emplace) are not like this; they can be adequately experimented with using only helper functions like from core-js’s pure mode. This is a rare condition.

  • The experimentation has to involve large codebases with many dependencies. Experimentation in smaller code snippets and code playgrounds is not affected. And, in my experience, most useful experimentation happens in code snippets and playgrounds, with only sporadic experimentation in big codebases with dependencies.

  • The experimentation has to use an NPM-published library that performs conditional global mutation with feature detection. But a developer who wishes to experiment does not have to use an NPM-published library that performs conditional global mutation with feature detection. They can use an NPM-published library that does not use feature detection, and they then can perform the global mutation themselves.

There are three conditions here, and all three seemingly have to be true for this pull request to be a problem.

The intersection of these three conditions seems like such a rare use case, compared to the experimentation that is not covered by all three of these conditions. Such an overwhelming majority of experimentation is still possible by any of these actions: (1) directly using a helper function, (2) playing with code snippets or playgrounds rather than large codebases with many dependencies, or (3) importing a library that does not conditionally mutate using feature detection and then doing the global mutation themselves (still without feature detection).

Even production code can safely (safe = doesn’t break if a proposal changes) mutate globals however it wants, as long as the mutation is not conditional on feature detection. The library-publication thing might be a non sequitur, in that production code that conditionally mutates globals (even without relying on a library) is unsafe, and libraries that mutate globals unconditionally without relying on feature detection are safe (as far as I can tell). A path forward for core-js could be that its global mode could switch to always mutating globals for proposals in Stages 0–2, rather than using feature detection.

But perhaps I am still missing something. My apologies if I am.

@zloirock
Copy link

zloirock commented Dec 18, 2021

@js-choi perfectly formulated.

The intersection of these three conditions seems like such a rare use case

Not so rare as you think and problematic not only the intersection case.

A path forward for core-js could be that its global mode could switch to always mutating globals for proposals in Stages 0–2, rather than using feature detection.

Yes, that makes sense. I'm thinking to do it for a long time.

@ptomato
Copy link

ptomato commented Jan 7, 2022

Thanks @js-choi for expressing the constraints. I would like to add my support for writing down this distinction of what exactly is harmful re. global mutation and feature detection and what isn't. Because it is quite a subtle point. For me, the Finding on Polyfills from the W3C TAG was really helpful when I became a TC39 delegate, but I had to have that document pointed out to me as it wasn't in a very discoverable place, at least for me.

Also note that "polyfill" has a specific narrow definition, but many JS programmers use the word casually with a much looser definition. I think seeing a thread pop up about "harmful polyfill recommendation" probably caused confusion in light of this. It certainly made me double-take when I first saw it.

Reacting to #33 (comment) - it is correct that having a Temporal polyfill (and here I am using the loose definition of the word polyfill, because it does not modify globals) did not catch all issues, but I also don't think it's accurate to say that it failed to catch most of them — I think the polyfill did actually directly allow us to catch many issues before Stage 3, orders of magnitude more than the few dozen issues we've presented to committee post Stage 3. Some of these were caught just by writing the polyfill and writing tests for it, but I believe publishing it on NPM was also important. We saw people using it for toy projects, or for performing speculative ports of their codebase without the intent of publishing it to production, and got really good feedback from that. I expect that had the Temporal polyfill not been published on NPM, the friction for trying it out would've been higher and we wouldn't have gotten as much participation.

Having said all that, I support the current wording of this PR that explicitly clarifies that reference implementations that don't modify globals may be published. I would additionally recommend adding a link to that TAG document (or a similar one that we write ourselves) to give more background to readers, since I personally would have benefited from reading that a couple of years ago.

@hax
Copy link
Member

hax commented Jan 18, 2022

I think the problem is not whether publishing to npm (u have to publish to npm if u want broader feedback), but how to clearly mark or even enforce them to be "experimental".

I wrote a rough and aggressive idea of Experimental API , which you can check at https://gist.github.com/hax/86a0817320985d046bebfb392bf9a49b .

@ljharb
Copy link
Member Author

ljharb commented Jan 18, 2022

You don’t have to publish something that modifies the environment to get that feedback, though, in the vast majority of cases - and even then, for feedback folks can install from GitHub if they really want.

@ChristianUlbrich
Copy link

In the current days of bundling and transpiling "installing from GitHub" becomes more and more difficult, as you'd typically won't commit build artifacts. Published stuff lowers the barrier for trying things out.

@ljharb
Copy link
Member Author

ljharb commented Jan 24, 2022

Today's TC39 provided consensus for the overall goal of clarifying the "expected implementation types" column - specifically to capture the original intent of stage 1 ("experimental") and to avoid web compatibility risks.

Precise text will be brought back to a future plenary for consensus. Some feedback from the committee include that we should try to be broad and not provide an exhaustive list of best practices; and we should link to https://www.w3.org/2001/tag/doc/polyfills/ as well, since it contains extensive research.

@@ -110,7 +110,7 @@ <h2>Stages</h2>
<td>The solution is complete and no further work is possible without implementation experience, significant usage and external feedback.
<td>Complete: all semantics, syntax and API are completed described
<td>Limited: only those deemed critical based on implementation experience
<td>Spec compliant
<td>Spec-compliant polyfills, engines, and any other implementations (implementations that modify global built-in objects may be tentatively published)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should reference the w3c on this: https://www.w3.org/2001/tag/doc/polyfills/#feature-life-cycle-and-the-role-of-polyfills and if this is ambiguous, highlight that polyfills are only recommended to be shipped after browsers have shipped. This covers a lot of the issues that come up.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I love the idea of linking here and also highlighting that recommendation, and that matches what we discussed in plenary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.