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

[babel-types] Fix isNodesEquivalent() behavior for TemplateElements #8165

Merged

Conversation

@timkendrick
Copy link
Contributor

timkendrick commented Jun 13, 2018

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

Bug Report

Current Behavior
Attempting to run t.isNodesEquivalent() on any node that contains a TemplateElement node somewhere in its subtree will throw an error.

Input Code

const t = require("babel-types");

const node = t.templateElement({ raw: 'foo', cooked: 'foo' }, true);

t.isNodesEquivalent(node, node);
// TypeError: Cannot convert undefined or null to object
//     at keys (<anonymous>)
//     at isNodesEquivalent (babel-types/lib/validators.js:223:35)

Solution

This error occurs because according to the NODE_FIELDS registry, TemplateElement defines a value property, which according to the ESTree spec is an object with { raw, cooked } properties – not a babel AST node.

The isNodesEquivalent() algorithm incorrectly assumes that any object properties must themselves be AST nodes, which is not the case here, causing the error.

As far as I can tell, the VISITOR_KEYS registry seems to be used elsewhere to determine which properties refer to sub-nodes of any given node. This means that the isNodesEquivalent() algorithm could potentially use this information to determine whether to compare the property values as AST nodes (i.e. recursively compare the values using isNodesEquivalent()) or as raw JS objects (i.e. compare the two objects via a shallow equality check).

Questions

  • I'm not hugely familiar with the babel-types source – is VISITOR_KEYS definitely the correct source of truth to use here? i.e. I've assumed that any property that is not present in the VISITOR_KEYS array will not contain an AST node. Is this a valid assumption?
@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Jun 13, 2018

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

1 similar comment
@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Jun 13, 2018

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

@hzoo
hzoo approved these changes Jul 5, 2018
@hzoo hzoo merged commit e9184ed into babel:master Jul 6, 2018
4 checks passed
4 checks passed
babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 80.54% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lock lock bot added the outdated 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.
Projects
None yet
6 participants
You can’t perform that action at this time.