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

Add test for TemplateStrings caching #1424

Merged
merged 2 commits into from
Feb 26, 2019

Conversation

jridgewell
Copy link
Contributor

@jridgewell jridgewell commented Feb 26, 2019

Safari 12 has a bug that breaks caching of TemplateStrings.

Test case: https://jsbin.com/leyusas/10/edit?js,console

Unfortunately, it's a memory GC bug, so it's very difficult to write a test that would catch this.

Re: babel/babel#9584

Safari 12 has a [bug](https://bugs.webkit.org/show_bug.cgi?id=190756) that breaks caching of `TaggedTemplateExpression`s.

Test case: https://jsbin.com/leyusas/10/edit?js,console

Unfortunately, it's a memory GC bug, so it's very difficult to write a test that would catch this.
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

LGTM but maybe we could add a note to the test linking to the bug, and explaining why the test might be tricky to verify?

@jridgewell
Copy link
Contributor Author

Done.

Copy link
Collaborator

@zloirock zloirock left a comment

Choose a reason for hiding this comment

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

Please, rebuild it one more time.

@jridgewell
Copy link
Contributor Author

Done.

@zloirock zloirock merged commit 6d012ba into compat-table:gh-pages Feb 26, 2019
@jridgewell jridgewell deleted the safari-template branch February 26, 2019 07:42
jridgewell added a commit to jridgewell/caniuse that referenced this pull request Feb 26, 2019
jridgewell added a commit to jridgewell/caniuse that referenced this pull request Feb 26, 2019
@moderndeveloperllc
Copy link

@jridgewell Just come upon this when updating to the babel 7.4 (which includes the compat-table build that fixes this). One of the things this fix means is that any build that targets 'last 2 Safari major versions', via browserlist syntax is now including the babelHelpers.taggedTemplateLiteral() where previously it was not. This can cause build pipeline issues with projects using a module/nomodule split as this is not normally needed for ES6 module-compatible browsers.

What did this bug cause to happen? Before I go and refudge my config, I was hoping to figure out if I really need to serve Safari 12 the helper-wrapped version or not. Thanks.

@jridgewell
Copy link
Contributor Author

The bug causes:

function strings(array) {
  return array;
}
function getStrings() {
  return strings`foo`;
}

const ref = getStrings();

// This should always be true
// It may not be in Safari
ref === getStrings();

Many libraries use the "always the same reference" property to do caching (via WeakMaps) of expensive work. If the references are not the same, then the caching is broken and the expensive work has to be redone. In some libraries, it can cause serious bugs.

@moderndeveloperllc
Copy link

@jridgewell Thanks for the explanation. It's unfortunate that babel preset-env currently can't handle the case of a feature having upper bounds in compat-table. BTW, you may want to add this issue to core-js as that is what babel is transitioning to it appears. There does not appear to be a template literals test in the list there.

jridgewell added a commit to jridgewell/compat-table that referenced this pull request May 6, 2019
compat-table#1424 for context.

The Safari bug revealed that `getStrings() !== new getStrings()`, making
this easily testable. I've separated the original test into two:

1. Test TemplateStrings call site [revision](tc39/ecma262#890)
2. Test the buggy Safari behavior explicitly
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.

4 participants