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

feat: Automatically generate cooked for templateElement #14757

Merged
merged 7 commits into from
Jul 21, 2022

Conversation

liuxingbaoyu
Copy link
Member

@liuxingbaoyu liuxingbaoyu commented Jul 14, 2022

Q                       A
Fixed Issues? Fixes #9242, Fixes #14682
Patch: Bug Fix? ?
Major: Breaking Change? ×
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes? add unraw
License MIT

I'm not sure if I need to check the cooked parameter with a value, please give your opinion.

Also I don't think we need to do the same in @babel/plugin-transform-template-literals, just update types.

@liuxingbaoyu liuxingbaoyu added PR: New Feature 🚀 A type of pull request used for our changelog categories pkg: types labels Jul 14, 2022
@babel-bot
Copy link
Collaborator

babel-bot commented Jul 14, 2022

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/52573/

Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

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


let cooked = null;
try {
cooked = unraw(raw);
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: should we throw if user provided inconsistent cooked? Currently we mute such errors and trust raw, but in this case either the raw or cooked could be incorrect.

Copy link
Member Author

Choose a reason for hiding this comment

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

should we throw if user provided inconsistent cooked?

I'm not sure.

Currently we mute such errors and trust raw, but in this case either the raw or cooked could be incorrect.

Can you give an example?
I test that fn \u is legal and \u is not.
Looks like it should be checked in TemplateLiteral.
Then we can do it later.😂

Copy link
Contributor

Choose a reason for hiding this comment

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

For example, when user provided both raw and cooked:

t.templateElement({
  raw: "face",
  cooked: "faec" // which one is correct? face or faec?
})

The current main behaviour is leave it as-is. The PR behaviour is to ignore cooked and reassign cooked to be raw. Now thanks to unraw we know that this is not a valid template element. Should we throw a validation error in this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Make this a behavior in babel8?
But I don't know if jest snap supports this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Now we ignore the cooked passed in by the user.
As for raw exceptions I think it can be done in the future.

@nicolo-ribaudo
Copy link
Member

We already have the logic to "unraw" template literals in our parser, I wonder if we could extract it from there to have a single implementation.

@liuxingbaoyu
Copy link
Member Author

@nicolo-ribaudo
The logic in parser is quite coupled, and keeping a single implementation seems difficult to do.
Actually I also tried to extract the code from parser at first, but didn't find it. 😓
I tried again just now and found it, but it looks like it's hard to even copy a copy of the code and modify it.

@liuxingbaoyu
Copy link
Member Author

@nicolo-ribaudo
I'm trying helper-string-parser and I'm having a little problem.
This way unterminatedCalled is true when raw is any value.

readStringContents("template", raw, 0, 0, 0, {
              unterminated() {
                unterminatedCalled = true;
              },
              strictNumericEscape: error,
              invalidEscapeSequence: error,
              numericSeparatorInEscapeSequence: error,
              unexpectedNumericSeparator: error,
              invalidDigit: error,
              invalidCodePoint: error,
            })

@nicolo-ribaudo
Copy link
Member

I think it's not true when raw is

abc`def

(which is actually an invalid raw, so we can throw if unterminatedCalled remains false)

@liuxingbaoyu
Copy link
Member Author

image

I'm not quite sure how I should use it.😕

@nicolo-ribaudo
Copy link
Member

Try this:

let unterminatedCalled = false;
let str, containsInvalid;
try {
  const error = () => {
    throw new Error();
  };
  ({ str, containsInvalid } = readStringContents(
    "template",
    "abc\\u{1000_0000}",
    0,
    0,
    0,
    {
      unterminated() {
        unterminatedCalled = true;
      },
      strictNumericEscape: error,
      invalidEscapeSequence: error,
      numericSeparatorInEscapeSequence: error,
      unexpectedNumericSeparator: error,
      invalidDigit: error,
      invalidCodePoint: error,
    }
  ));
} catch {
  // TODO: When https://github.com/babel/babel/issues/14775 is fixed
  // we can remove the try/catch block.
  unterminatedCalled = true;
  containsInvalid = true;
}

if (!unterminatedCalled) throw new Error("Invalid raw");

cooked = containsInvalid ? null : str;

some tests:

"abcdef"; // ok
"abc\u{123}def"; // ok
"abc\u{}def"; // null cooked
"abc\u{10000_0000}def"; // null cooked
"abc`def"; // Invalid raw, error
"abc`"; // Invalid raw, error
"`abc"; // Invalid raw, error
"abc${"; // Invalid raw, error
"${abc"; // Invalid raw, error
"abc\\`"; // ok

@liuxingbaoyu
Copy link
Member Author

liuxingbaoyu commented Jul 20, 2022

image

It's weird.
Is str being '' expected?

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Jul 20, 2022

Whops sorry, it's a bug in helper-string-parser (that does not affect @babel/parser): can you add out += input.slice(chunkStart, pos); after the first error.unterminated?

@liuxingbaoyu
Copy link
Member Author

Now it works fine!

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

Thank you! I just added two new tests.

As a Babel 8 follow-up, we could always try to generate .cooked and throw if the generated version is different from the one passed as an argument.

@liuxingbaoyu liuxingbaoyu changed the title feat: Automatically generate cooked for templateElement. feat: Automatically generate cooked for templateElement Jul 21, 2022
@liuxingbaoyu liuxingbaoyu merged commit 32d4f6e into babel:main Jul 21, 2022
@victor-wm
Copy link

@liuxingbaoyu do you know when this is going to be released?

@liuxingbaoyu
Copy link
Member Author

I'm not sure, generally released by @nicolo-ribaudo and @JLHwung .

@nicolo-ribaudo
Copy link
Member

I can release later today!

@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Nov 1, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: types PR: New Feature 🚀 A type of pull request used for our changelog categories
Projects
None yet
5 participants