This repository has been archived by the owner. It is now read-only.

Fix source location for JSXEmptyExpression nodes (fixes #248) #249

Merged
merged 1 commit into from Dec 14, 2016

Conversation

Projects
None yet
4 participants
@jlongster
Copy link
Contributor

jlongster commented Dec 8, 2016

Q A
Bug fix? yes
Breaking change? no
New feature? no
Deprecations? no
Spec compliancy? no
Tests added/pass? no
Fixed tickets #248
License MIT

Turned out this is an easy fix. Looks like it wasn't appropriately using this.state as it should. See #248.

@hzoo hzoo added the Tag: Bug Fix label Dec 8, 2016

@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Dec 8, 2016

👋 @jlongster - can you also add a fixture test for this?

@jlongster

This comment has been minimized.

Copy link
Contributor Author

jlongster commented Dec 8, 2016

Sure, need to learn how to add a test first so will take me a bit.

@DrewML

This comment has been minimized.

Copy link
Member

DrewML commented Dec 11, 2016

@jlongster All of the tests in Babylon just have fixtures that match a source string to the serialized AST. Basic steps are:

  1. Add a new directory with a descriptive name under one of the subfolders in test/fixtures/jsx
  2. Add an actual.js file in that folder with the code to use as a test case
  3. Run npm run test-only, which will automatically generate an expected.json for you
  4. Inspect the AST output to expected.json to verify it looks correct

It also looks like you'll need to regenerate the results for test/fixtures/jsx/basic/10, since they're currently committed without the proper location info. You can delete that expected.json and re-run npm run test-only to regenerate.

@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Dec 12, 2016

We can add this steps in CONTRIBUTING like I did in Babel via https://github.com/babel/babel/blob/master/CONTRIBUTING.md#writing-tests

@jlongster

This comment has been minimized.

Copy link
Contributor Author

jlongster commented Dec 13, 2016

@hzoo The current tests seem to be failing:

% npm run test-only

> babylon@6.14.1 test-only /Users/james/projects/babylon
> ava


  2343 passed
  1 failed

  jsx/basic/10
  Error: /Users/james/projects/babylon/test/fixtures/jsx/basic/10/actual.js: [
  {
    "type": "CommentBlock",
    "value": " this is a comment ",
    "start": 4,
    "end": 27,
    "loc": {
      "start": {
        "line": 1,
        "column": 4
      },
      "end": {
        "line": 1,
        "column": 27
      }
    }
  }
] !== null (leadingComments/expression/0/children/expression/0/body/program)
    runTest (test/utils/runFixtureTests.js:62:13)
    Test.fn (test/utils/runFixtureTests.js:14:22)

This is on the latest master without my changes. I see this failure in my CI too. I suppose I can fix it along with my PR, but is that expected? I don't see that failure in other PRs, but I don't understand why I can reproduce it by checking out master without my changes.

@DrewML

This comment has been minimized.

Copy link
Member

DrewML commented Dec 13, 2016

I don't understand why I can reproduce it by checking out master without my changes.

@jlongster It's not super well documented (or at all?), but the tests right now actually run against the artifact spit out by Rollup. We have some clean up to do there.

For the time being, try running npm run build && npm run test-only on master, and things should be passing.

Regarding the current tests failing in CI, haven't looked deep into it yet, but my suspicion is that your fix addressed an issue that we didn't notice when that fixture was last checked in. If you verify the AST in jsx/basic/10 is correct (and that it was wrong before) you can delete the expected.json and run npm run test-only to generate the new fixture to include in your PR.

Edit: Yeah, the fixture checked in had some issues. Empty object for loc, and no start or end prop on the JSXEmptyExpression node. Just regenerate that fixture and you should be set.

Thanks for contributing, btw!

@jlongster

This comment has been minimized.

Copy link
Contributor Author

jlongster commented Dec 13, 2016

That makes sense, I figured it was something like that! No worries, keeping docs up-to-date and making these kinds of things takes a huge amount of time so I don't really mind figuring some things out.

@jlongster jlongster force-pushed the jlongster:master branch from 593426e to 5df6141 Dec 13, 2016

@jlongster

This comment has been minimized.

Copy link
Contributor Author

jlongster commented Dec 13, 2016

Done!

@DrewML

DrewML approved these changes Dec 13, 2016

@danez

danez approved these changes Dec 14, 2016

@danez

This comment has been minimized.

Copy link
Member

danez commented Dec 14, 2016

Nice catch, thanks.

@jlongster

This comment has been minimized.

Copy link
Contributor Author

jlongster commented Dec 14, 2016

Any chance this could be merged?

@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Dec 14, 2016

👍 thanks @jlongster

@hzoo hzoo merged commit ba96b91 into babel:master Dec 14, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.