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

Reuse same object for tagged template quasis. #67

Merged
merged 2 commits into from
Dec 6, 2017

Conversation

goto-bus-stop
Copy link
Contributor

@goto-bus-stop goto-bus-stop commented Dec 5, 2017

Spec tagged template literals pass the same object to each invocation of
the template tag function. This way that object can be used as a cache
key, for example, using a Map, so that parsing of the string contents
can be cached.

This patch changes the tagged template literal transform to declare a
frozen top-level array with the quasi strings, and passes that array to
each invocation. If this lands I think it's important to adhere to the spec re:
freezing, because otherwise people might try to attach properties to the
object itself as a sort of caching mechanism, which doesn't work in spec
tagged template literals.

I'm unsure if this is the best way to declare a top-level variable,
but it seems to work OK :)

Spec tagged template literals pass the same object to each invocation of
the template tag function. This way that object can be used as a cache
key, for example, using a `Map`, so that parsing of the string contents
can be cached.

This patch changes the tagged template literal transform to declare a
frozen top-level array with the quasi strings, and passes that array to
each invocation. I think it's important to adhere to the spec re:
freezing, because otherwise people might try to attach properties to the
object itself as a sort of caching mechanism, which doesn't work in spec
tagged template literals.

I'm unsure if this is the _best_ way to declare a top-level variable,
but it seems to work OK :)
@adrianheine
Copy link
Member

At least my Node v6.12.0 and my Firefox 57 seem to intern the arrays, so that even different call sites pass the same object:

let x;
function f(r) { x = x || r; console.log(x === r) };
f`a${ 1 }`; // true
f`a${ x }`; // true
f`b`; // false
f(["a"]); // false
f`a${ 5 }` // true

@goto-bus-stop
Copy link
Contributor Author

oh yeah that's a great catch, thanks! 6272cc0 only creates a new templateObject if one with identical quasis doesn't exist yet in the file.

@adrianheine
Copy link
Member

Wow, that was fast! This doesn't work across multiple files, but otherwise it's great, and I doubt there is a reasonable way to make that work.

@goto-bus-stop
Copy link
Contributor Author

yeah, exactly—and even so, this will still allow template tags to save a lot of time by reducing parsing to just once per template per file, instead of on every single call.

@adrianheine adrianheine merged commit 050480c into bublejs:master Dec 6, 2017
@adrianheine
Copy link
Member

Ok, I just went ahead and merged this, it's definitely an improvement :) Thanks for your contribution!

@adrianheine
Copy link
Member

I just pushed 7583ae8, which fixed this code putting definitions before directives.

@goto-bus-stop
Copy link
Contributor Author

oops! thanks!

goto-bus-stop added a commit to stackcss/sheetify that referenced this pull request Jun 27, 2018
bublejs/buble#67 changed Bublé template strings to compile similarly to Babel ones, for spec reasons.
With this patch, the new style as well as the old style are detected correctly.
@goto-bus-stop goto-bus-stop deleted the template-object branch August 28, 2018 14:10
@adrianheine
Copy link
Member

Actually, Runtime Semantics: GetTemplateObject step 4.a specifies that each parse node gets its own template object, contrary to what V8 does. See also tc39/test262#972.

@goto-bus-stop
Copy link
Contributor Author

O, that's nice. I can prepare a PR that basically reverts the second commit from this one today, I think.

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.

2 participants