Skip to content

[proposal-object-rest-spread] fix templateLiteral in extractNormalizedKeys #9628

Merged
nicolo-ribaudo merged 7 commits intobabel:masterfrom
pnowak:master
Mar 13, 2019
Merged

[proposal-object-rest-spread] fix templateLiteral in extractNormalizedKeys #9628
nicolo-ribaudo merged 7 commits intobabel:masterfrom
pnowak:master

Conversation

@pnowak
Copy link
Contributor

@pnowak pnowak commented Mar 3, 2019

Q                       A
Fixed Issues? #9563
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

Added additional check in extractNormalizedKeys function to handle templates differently than string.

@babel-bot
Copy link
Collaborator

babel-bot commented Mar 3, 2019

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

// since a key {a: 3} is equivalent to {"a": 3}, use the latter
keys.push(t.stringLiteral(prop.key.name));
} else if (t.isTemplateLiteral(prop.key)) {
keys.push(t.templateLiteral(prop.key.quasis, prop.key.expressions));
Copy link
Member

Choose a reason for hiding this comment

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

Since this is the same as doing t.cloneNode(prop.key), you can use that function. Note that this shouldn't set allLiteral to false (because templates are always strings), so it shouldn't be merged with the last else block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think something like check isLiteral and not isTemplateLiteral?

given_name: givenName,
'last_name': lastName,
[`country`]: country,
[prefix + 'state']: state,
Copy link
Member

Choose a reason for hiding this comment

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

Could you also add a test without this line, so that allLiterals will be true?

Copy link
Contributor Author

@pnowak pnowak Mar 4, 2019

Choose a reason for hiding this comment

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

Ok, I will remove this BinaryExpression.

Copy link
Member

Choose a reason for hiding this comment

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

Could you add this test, to test the last changes?

@nicolo-ribaudo nicolo-ribaudo added PR: Bug Fix 🐛 A type of pull request used for our changelog categories Spec: Object Rest/Spread labels Mar 4, 2019
// since a key {a: 3} is equivalent to {"a": 3}, use the latter
keys.push(t.stringLiteral(prop.key.name));
} else if (t.isLiteral(prop.key)) {
} else if (t.isLiteral(prop.key) && !t.isTemplateLiteral(prop.key)) {
Copy link
Member

Choose a reason for hiding this comment

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

This will set allLiterals to false for template literals, but it isn't needed.

You can do something like

Suggested change
} else if (t.isLiteral(prop.key) && !t.isTemplateLiteral(prop.key)) {
} else if (t.isTemplateLiteral(prop.key)) {
keys.push(t.cloneNode(prop.key));
} else if (t.isLiteral(prop.key)) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank You very much.

@nicolo-ribaudo
Copy link
Member

I meant to create a new test, to test both the allLiterals: true and allLiterals: false cases (possibly in two different files):

const {
  given_name: givenName,
  'last_name': lastName,
  [`country`]: country,
  [prefix + 'state']: state,
  [`${prefix}consents`]: consents,
  ...rest
} = input;
const {
  given_name: givenName,
  'last_name': lastName,
  [`country`]: country,
  ...rest
} = input;

@nicolo-ribaudo
Copy link
Member

Thank you!

@nicolo-ribaudo nicolo-ribaudo merged commit a64bf63 into babel:master Mar 13, 2019
@pnowak
Copy link
Contributor Author

pnowak commented Mar 14, 2019

Hi @nicolo-ribaudo.
Thank you for your help and the solution ;)

mAAdhaTTah added a commit to mAAdhaTTah/babel that referenced this pull request Mar 15, 2019
* master: (58 commits)
  Remove dependency on home-or-tmp package (babel#9678)
  [proposal-object-rest-spread] fix templateLiteral in extractNormalizedKeys (babel#9628)
  Partial application plugin (babel#9474)
  Private Static Class Methods (Stage 3) (babel#9446)
  gulp-uglify@3.0.2
  rollup@1.6.0
  eslint@5.15.1
  jest@24.5.0
  regexpu-core@4.5.4
  Remove input and length from state (babel#9646)
  Switch from rollup-stream to rollup and update deps (babel#9640)
  System modules - Hoist classes like other variables (babel#9639)
  fix: Don't transpile ES2018 symbol properties (babel#9650)
  Add WarningsToErrorsPlugin to webpack to avoid missing build problems on CI (babel#9647)
  Update regexpu-core dependency (babel#9642)
  Add placeholders support to @babel/types and @babel/generator (babel#9542)
  Generate plugins file
  Make babel-standalone an ESModule and enable flow (babel#9025)
  Reorganize token types and use a map for them (babel#9645)
  [TS] Allow context type annotation on getters/setters (babel#9641)
  ...
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019
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 PR: Bug Fix 🐛 A type of pull request used for our changelog categories Spec: Object Rest/Spread

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants