Visitar URL original
fix: tagged template incorrect receiver by sag1v · Pull Request #13395 · babel/babel · GitHub
Skip to content

fix: tagged template incorrect receiver - #13395

Merged
JLHwung merged 6 commits into
babel:mainfrom
sag1v:tagged-private-field-this
Jun 15, 2021
Merged

fix: tagged template incorrect receiver#13395
JLHwung merged 6 commits into
babel:mainfrom
sag1v:tagged-private-field-this

Conversation

@sag1v

@sag1v sag1v commented May 29, 2021 •
edited by nicolo-ribaudo
Loading

Copy link
Copy Markdown
Contributor
Q                       A
Fixed Issues? Fixes #13366
Patch: Bug Fix? 👍
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? 👍
Documentation PR Link
Any Dependency Changes?
License MIT

@sag1v sag1v mentioned this pull request May 29, 2021
Closed
1 task
@babel-bot

babel-bot commented May 29, 2021 •
edited
Loading

Copy link
Copy Markdown
Collaborator

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

@codesandbox-ci

codesandbox-ci Bot commented May 29, 2021 •
edited
Loading

Copy link
Copy Markdown

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 9a0d7ec:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@nicolo-ribaudo nicolo-ribaudo added PR: Bug Fix 🐛 A type of pull request used for our changelog categories Spec: Class Fields Spec: Private Methods labels May 29, 2021
nicolo-ribaudo
nicolo-ribaudo reviewed May 29, 2021
View reviewed changes
Comment thread .../babel-plugin-proposal-class-properties/test/fixtures/private/tagged-template-static/exec.js Outdated
Comment thread .../babel-plugin-proposal-class-properties/test/fixtures/private/tagged-template-static/exec.js Outdated
Comment thread ...l-plugin-proposal-class-properties/test/fixtures/private/tagged-template-static/options.json Outdated
Comment thread .../babel-plugin-proposal-private-methods/test/fixtures/private-method/tagged-template/input.js Outdated
Comment thread .../babel-plugin-proposal-private-methods/test/fixtures/static-accessors/tagged-template/exe.js Outdated
@sag1v

sag1v commented May 29, 2021

Copy link
Copy Markdown
Contributor Author

@nicolo-ribaudo Thanks for the comments.

General question, Can i create a new commit (fixing review) and push to this PR or do i must keep it with a single commit (using commit --amend with force push)?

jridgewell
jridgewell requested changes May 29, 2021
View reviewed changes
Comment thread packages/babel-helper-member-expression-to-functions/src/index.ts Outdated
Comment thread .../babel-plugin-proposal-class-properties/test/fixtures/private/tagged-template-static/exec.js Outdated
Comment thread packages/babel-plugin-proposal-private-methods/test/fixtures/accessors/tagged-template/exec.js Outdated
@jridgewell

jridgewell commented May 29, 2021

Copy link
Copy Markdown
Member

It's fine to add as many commits as needed to this PR.

sag1v
sag1v commented May 29, 2021
View reviewed changes
Comment thread ...babel-plugin-proposal-class-properties/test/fixtures/private/tagged-template-static/input.js Outdated
@sag1v

sag1v commented May 29, 2021

Copy link
Copy Markdown
Contributor Author

@nicolo-ribaudo @jridgewell I pushed the new changes per your comments

@sag1v

sag1v commented Jun 1, 2021

Copy link
Copy Markdown
Contributor Author

@nicolo-ribaudo / @jridgewell Are we done here? :)

nicolo-ribaudo
nicolo-ribaudo reviewed Jun 1, 2021
View reviewed changes

@nicolo-ribaudo nicolo-ribaudo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I left a few more comments about tests; apart from them it looks good 👍

Comment thread .../babel-plugin-proposal-class-properties/test/fixtures/private/tagged-template-static/exec.js
Comment thread packages/babel-plugin-proposal-class-properties/test/fixtures/private/tagged-template/input.js Outdated
Comment thread packages/babel-plugin-proposal-private-methods/test/fixtures/accessors/tagged-template/exec.js
Comment thread ...abel-plugin-proposal-private-methods/test/fixtures/static-accessors/tagged-template/input.js Outdated
jridgewell
jridgewell reviewed Jun 1, 2021
View reviewed changes
Comment thread packages/babel-plugin-proposal-class-properties/test/fixtures/private/tagged-template/exec.js Outdated
Comment thread packages/babel-plugin-proposal-class-properties/test/fixtures/private/tagged-template/exec.js Outdated
Comment thread ...babel-plugin-proposal-class-properties/test/fixtures/private/tagged-template-static/input.js Outdated
Comment thread packages/babel-plugin-proposal-private-methods/test/fixtures/accessors/tagged-template/exec.js
Comment thread packages/babel-plugin-proposal-private-methods/test/fixtures/accessors/tagged-template/input.js
Comment thread ...s/babel-plugin-proposal-private-methods/test/fixtures/private-method/tagged-template/exec.js Outdated
Comment thread ...plugin-proposal-private-methods/test/fixtures/private-static-method/tagged-template/input.js Outdated
Comment thread ...abel-plugin-proposal-private-methods/test/fixtures/static-accessors/tagged-template/input.js Outdated
@sag1v
sag1v requested a review from jridgewell June 12, 2021 10:54
jridgewell
jridgewell reviewed Jun 14, 2021
View reviewed changes

@jridgewell jridgewell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry, just a few more comments (that I screwed up). If you can make these changes, run update the tests one last time, this should be good to merge.

Comment thread packages/babel-plugin-proposal-private-methods/test/fixtures/accessors/tagged-template/exec.js Outdated
Comment thread packages/babel-plugin-proposal-private-methods/test/fixtures/accessors/tagged-template/input.js
Comment thread ...abel-plugin-proposal-private-methods/test/fixtures/static-accessors/tagged-template/input.js Outdated
@sag1v

sag1v commented Jun 15, 2021

Copy link
Copy Markdown
Contributor Author

@jridgewell Thanks, done.

existentialism
existentialism approved these changes Jun 15, 2021
View reviewed changes

@existentialism existentialism left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@JLHwung
JLHwung merged commit a3c7497 into babel:main Jun 15, 2021
@sync-by-unito sync-by-unito Bot mentioned this pull request Jun 22, 2021
Closed
@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 Sep 15, 2021
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Sep 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@nicolo-ribaudo nicolo-ribaudo nicolo-ribaudo left review comments
@existentialism existentialism existentialism approved these changes
@jridgewell jridgewell jridgewell approved these changes
@JLHwung JLHwung JLHwung approved these changes

Assignees

No one assigned

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: Class Fields Spec: Private Methods

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: Tagged template with strict private field/method tag has incorrect receiver

6 participants

@sag1v @babel-bot @jridgewell @existentialism @JLHwung @nicolo-ribaudo