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

Forward deopt node path #11832

Merged
merged 3 commits into from Aug 20, 2020
Merged

Forward deopt node path #11832

merged 3 commits into from Aug 20, 2020

Conversation

johanholmerin
Copy link
Contributor

@johanholmerin johanholmerin commented Jul 13, 2020

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

Currently, when evaluating array values, object keys, and object properties, if a deopt is found, the location is discarded and the location in the array or the object will be used. I suggest this information is kept. It will make it easier to understand the cause of the deopt.

{
  a: { // <- deopt currently points to this object
    b // <- I suggest pointing to this identifier
  }
}

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 13, 2020

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 c3065e8:

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

@babel-bot
Copy link
Collaborator

babel-bot commented Jul 13, 2020

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

@@ -21,6 +21,25 @@ describe("evaluation", function () {
});
}

function addDeoptTest(code, type, expectedType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Can you move these tests to packages/babel-traverse/test/evaluation.js?

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.

Can you also update packages/babel-traverse/src/path/evaluation.js#L410? The deopt option is not documented in the output interface.

@JLHwung JLHwung added the PR: Polish 💅 A type of pull request used for our changelog categories label Jul 15, 2020
@johanholmerin
Copy link
Contributor Author

Fixed

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.

Thanks.

@johanholmerin
Copy link
Contributor Author

Do you need anything from me before merging?

@JLHwung
Copy link
Contributor

JLHwung commented Aug 16, 2020

@johanholmerin We have a two-approval policy on merging PR. ping @babel/core-team.

Copy link
Member

@hzoo hzoo left a comment

Choose a reason for hiding this comment

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

If this is just restoring lost info (the path) sounds good, was just wondering how this may break existing usage if at all

@JLHwung JLHwung merged commit b9407d7 into babel:main Aug 20, 2020
@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 20, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 20, 2020
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: Polish 💅 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants