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

React-docgen fails to parse components with optional chaining. #463

Closed
mjhenkes opened this issue Jul 31, 2020 · 21 comments · Fixed by #468
Closed

React-docgen fails to parse components with optional chaining. #463

mjhenkes opened this issue Jul 31, 2020 · 21 comments · Fixed by #468

Comments

@mjhenkes
Copy link
Contributor

mjhenkes commented Jul 31, 2020

If a component contains optional chaining it will now fail to parse. This used to work.

I'm on version 4.1.1.

Now you get this error:

Error: did not recognize object of type "ChainExpression"
    at Object.getFieldNames (/proj/node_modules/ast-types/lib/types.js:661:19)
    at visitChildren (/proj/node_modules/ast-types/lib/path-visitor.js:186:36)
    at Visitor.PVp.visitWithoutReset (/proj/node_modules/ast-types/lib/path-visitor.js:168:20)
    at visitChildren (/proj/node_modules/ast-types/lib/path-visitor.js:205:25)
    at Visitor.PVp.visitWithoutReset (/proj/node_modules/ast-types/lib/path-visitor.js:168:20)
    at NodePath.each (/proj/node_modules/ast-types/lib/path.js:89:26)
    at visitChildren (/proj/node_modules/ast-types/lib/path-visitor.js:180:18)
    at Visitor.PVp.visitWithoutReset (/proj/node_modules/ast-types/lib/path-visitor.js:168:20)
    at visitChildren (/proj/node_modules/ast-types/lib/path-visitor.js:205:25)
    at Visitor.PVp.visitWithoutReset (/proj/node_modules/ast-types/lib/path-visitor.js:168:20)

I kind of suspect this has to do with one of the transitive dependencies more that react-docgen but I'm logging this here first.

How to recreate, run the following test against the below jsx file.

test.js

const reactDocs = require('react-docgen');
const fs = require('fs');

const filePath = './file.jsx';

const source = fs.readFileSync(filePath, 'utf8');

const parsedProps = reactDocs.parse(source);

console.log('parsed', parsedProps);

file.jsx

import React from 'react';

const ApplicationBase = () => {
  const thing = {
    stuff: 'stuff',
  };

  // Parse works when the next line is commented out.
  const string = thing?.stuff;
  // Works when un-commented
  // const string = 'stuff';

  return (
    <div>
      {string}
    </div>
  );
};

export default ApplicationBase;

Also an issue template would help me remember all the things you'd like to see on an issue 😄

@mjhenkes mjhenkes changed the title React-docgen fails to parse componets with optional chaining. React-docgen fails to parse components with optional chaining. Jul 31, 2020
@mjhenkes
Copy link
Contributor Author

I think this is likely related to @babel/[email protected] which released yesterday
https://www.npmjs.com/package/@babel/parser

@mjhenkes
Copy link
Contributor Author

mjhenkes commented Aug 3, 2020

Here is a repo re-creating this issue: https://github.com/mjhenkes/react-doc-gen-test

@mjhenkes
Copy link
Contributor Author

mjhenkes commented Aug 3, 2020

Looks like babel parser estree plugin made a change that is incompatible with ast-types
I've logged this issue babel/babel#11908 to babel to check if this considered a non passive change or if it's a bug that ast-types doesn't support the chainedExpression type.

@mjhenkes
Copy link
Contributor Author

mjhenkes commented Aug 3, 2020

You can work around this problem by locking in your babel dependencies in your project's package.json

For example:

  "devDependencies": {
-    "@babel/core": "^7.5.0",
+    "@babel/core": "7.10.5",
+    "@babel/parser": "7.10.5",

Here's the change we made to our project:
https://github.com/cerner/terra-ui/pull/290/files

@mjhenkes
Copy link
Contributor Author

mjhenkes commented Aug 3, 2020

Update: Babel says it's an intentional change. I've logged an issue to ast-types to add support for the chainExpression type.

@nafg
Copy link

nafg commented Aug 16, 2020

@mjhenkes can you link the issue

@mjhenkes
Copy link
Contributor Author

@nafg Sorry, I should have added that to my previous reply. benjamn/ast-types#383

@eps1lon
Copy link
Contributor

eps1lon commented Sep 7, 2020

On a different note: Should the AST even be handled by ast-types and not by @babel/types? Seems like babel is not tied to the AST spec that ast-types is using.

@jquense
Copy link
Collaborator

jquense commented Sep 8, 2020

There isn't any need to use AST-types any more I don't think

@info-bit
Copy link

As I can see ast-types released a version to support the new ChainExpression. Should react-docgen update his internal dependency to close the issue?

@ethancrook99
Copy link

+1 for using the new version of ast-types

@brianespinosa
Copy link

@danez it appears that the above referenced PR resolves this issue with the new version of ast-types. I looked at the commit history and except for the above PR that resolves this, and a doc update, all of the commits since the last release seem to be Dependabot updates.

Based on this info, what are the chances we can get a patch or minor version release published since this particular issue is breaking a number of other projects, as seen from incoming references? Also, let me know if there is any way I can assist with this in case there is any work needed to evaluate the version bumps on dependencies to understand what our semantic version should be.

@lucantini
Copy link

Hi!

When can we expect a new release from react-docgen with these changes?

@jquense
Copy link
Collaborator

jquense commented Sep 24, 2020

ping

@eps1lon
Copy link
Contributor

eps1lon commented Sep 24, 2020

Note that you can't fix this in your project without using e.g. resolutions in yarn. Package managers treat 0.x minor bumps as major and won't use [email protected] for react-docgen even on fresh installations. So even if this is just a minor version bump it still requires a new release.

@Brianzchen
Copy link

Note that you can't fix this in your project without using e.g. resolutions in yarn. Package managers treat 0.x minor bumps as major and won't use [email protected] for react-docgen even on fresh installations. So even if this is just a minor version bump it still requires a new release.

In case anyone is interested in what this means, in your package.json just add a resolutions key like so

  "resolutions": {
    "ast-types": "^0.14.0"
  },

@idbartosz
Copy link

bump, @danez when we can expect a release containing this update?

@jquense
Copy link
Collaborator

jquense commented Oct 21, 2020

please share the maintenance burden with more people. many projects rely on this project and folks would be happy to help out. It's totally understandable @danez to not have a ton of time to devote to this project, please enable others to share the burden.

@danez
Copy link
Collaborator

danez commented Oct 21, 2020

I just did a release, I can't change the situation, I don't have admin access.

@jquense
Copy link
Collaborator

jquense commented Oct 21, 2020

@gaearon or @bvaughn can we give @danez here admin access to this repo?

@bvaughn
Copy link

bvaughn commented Oct 21, 2020

Sure. Done.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.