Revert "Update packages/template/template.js"

romainmenke commented 2 years ago

This reverts commit 041c940a9333f00a2386f8a2951d918dd5e89c28.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals

Template literals are not supported in older browsers. This reverts a commit where template literals were added.

romainmenke commented 2 years ago

@bicknellr @aomarks Any chance to get this reviewed and shipped?

bicknellr commented 1 year ago

(relaying from #515) Although the npm packages for the polyfills include original sources (JS, at least), they're not guaranteed to have an upper bound on the ES version. If you're running any of these polyfills in a browser, you should use the minified bundles: those are compiled down to ES5, so you won't have to worry about template strings there.

romainmenke commented 1 year ago

Although the npm packages for the polyfills include original sources (JS, at least), they're not guaranteed to have an upper bound on the ES version. If you're running any of these polyfills in a browser, you should use the minified bundles: those are compiled down to ES5, so you won't have to worry about template strings there.

Can you reference any documentation where this is mentioned? I could not find it :/

The inclusion of template strings was a breaking change but this wasn't mentioned in release logs and was pushed as minor/patch bump. There was no technical reason to use template strings there (afaik) and reverting that change is just a low effort way to resolve real issues for users.

Having an unminified source for polyfill.io is also a useful debugging tool for users. Easier to provide a stack trace and inspect the code.

This is not possible if we start from a minified source.

bicknellr commented 1 year ago

Yeah, I can't find any documentation that alludes to this and in fact we have a page on lit.dev that specifically points says the opposite. :( I'll make a PR for this.

bicknellr commented 1 year ago

w.r.t. stack traces: I found this page on the React Native site that talks about a symbolication tool they made that seems to do the trick here. It takes a source map as an argument and arbitrary text over standard in, then it converts things that look like "file_path:123:456" and replaces them with the result of looking them up through the given source map. We distribute source maps along with the minified bundles.

For example:

<!DOCTYPE html>
<script>
  (window.customElements = window.customElements || {}).forcePolyfill = true;
</script>
<script src="https://unpkg.com/@webcomponents/custom-elements@1.5.0/custom-elements.min.js"></script>
<script>
  class TestElement extends HTMLElement {
    static get observedAttributes() {
      throw new Error('the error');
    }
    attributeChangedCallback() {
      // Do nothing.
    }
  }
  customElements.define('test-element', TestElement);
</script>
<test-element></test-element>

That page causes this error to be logged:

Uncaught Error: the error
    at get observedAttributes [as observedAttributes] ((index):10:13)
    at Aa (custom-elements.min.js:30:431)
    at Y.define (custom-elements.min.js:29:154)
    at (index):16:18

We can send that into metro-symbolicate and get the original locations:

$ curl https://unpkg.com/@webcomponents/custom-elements@1.5.0/custom-elements.min.js.map -o custom-elements.min.js.map
$ npm install metro-symbolicate
$ npx metro-symbolicate custom-elements.min.js.map <<EOF
> Uncaught Error: the error
>     at get observedAttributes [as observedAttributes] ((index):10:13)
>     at Aa (custom-elements.min.js:30:431)
>     at Y.define (custom-elements.min.js:29:154)
>     at (index):16:18
> EOF
Uncaught Error: the error
    at get observedAttributes [as observedAttributes] ((null:null:null)
    at Aa (ts_src/CustomElementRegistry.ts:163:null)
    at Y.define (ts_src/CustomElementRegistry.ts:97:internal_reifyDefinition)
    at (ts_src/Utilities.ts:84:null

It looks like it attempts to map everything it thinks is a file:row:col reference and assumes slightly different syntax, so some of them are broken, but the ones that actually are in the minified bundle are mapped correctly.

bicknellr commented 1 year ago

PR for the docs update: https://github.com/lit/lit.dev/pull/895

webcomponents / polyfills

Revert "Update packages/template/template.js" #476