Skip to content

Babel/types/fix/export flow types#11671

Closed
kayhadrin wants to merge 4 commits intobabel:mainfrom
kayhadrin:babel/types/fix/export-flow-types
Closed

Babel/types/fix/export flow types#11671
kayhadrin wants to merge 4 commits intobabel:mainfrom
kayhadrin:babel/types/fix/export-flow-types

Conversation

@kayhadrin
Copy link
Copy Markdown

@kayhadrin kayhadrin commented Jun 4, 2020

Q                       A
Fixed Issues? This could help #8739
Patch: Bug Fix? Bug fix
Major: Breaking Change? No, but improved types could reveal new Flow errors
Minor: New Feature?
Tests Added + Pass? Tests OK;
Documentation PR Link
Any Dependency Changes? No
License MIT

To properly import @babel/types types in a Flow+JS project, we need to fix a few things:

  1. Declare that the @babel/types module exports a default value; otherwise, we'll get a Flow error that sounds like "cannot import a default export because there is no default export"
  2. Explicitly export each function and type of the @babel/types module
  3. Fix existing Flow typing errors. E.g. Node -> BabelNode
  4. Add the @flow pragma in the file header. It's not mandatory, but it helps in some scenarios when we want to import the index.js.flow file directly.

Generated output example: https://gist.github.com/kayhadrin/dfe0f9785622a1dfaf726cba0001b993

@kayhadrin kayhadrin marked this pull request as ready for review June 4, 2020 12:41
@babel-bot
Copy link
Copy Markdown
Collaborator

babel-bot commented Jun 4, 2020

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

@codesandbox-ci
Copy link
Copy Markdown

codesandbox-ci Bot commented Jun 4, 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 b153d07:

Sandbox Source
hungry-brook-bfnxb Configuration
eager-brook-dip73 Configuration

@nicolo-ribaudo
Copy link
Copy Markdown
Member

Declare that the @babel/types module exports a default value; otherwise, we'll get a Flow error that sounds like "cannot import a default export because there is no default export"

That error seems correct, since @babel/types doesn't have a default export. You should use it as import * as t from "@babel/types".

@kayhadrin
Copy link
Copy Markdown
Author

kayhadrin commented Jun 6, 2020

@nicolo-ribaudo

That error seems correct, since @babel/types doesn't have a default export. You should use it as import * as t from "@babel/types".

Your recommendation is in plain JS and suggests to import all the exported JS values of a module, but I'm trying to import the exported default type of a JS module as a Flow type.

In Flow, that's done like this: import typeof MyModuleType from 'MyModule'
and that's when the Flow error mentioned earlier is outputted: "cannot import a default export because there is no default export".

Hence why I needed to do some of the changes in this PR.
Beside that, there are several other Flow typing errors that were fixed too.

@kayhadrin
Copy link
Copy Markdown
Author

🏓

@nicolo-ribaudo
Copy link
Copy Markdown
Member

?

import typeof * as t from "@babel/types"

kayhadrin added a commit to kayhadrin/fbt that referenced this pull request Jun 13, 2020
Summary:
For some strange reason, importing babel types with the syntax `import typeof * as BabelTypes from 'babel/types'` causes Flow errors to appear.
But if we use `import typeof as BabelTypes...` instead, it works as expected.

```
$ /oss-fbt/__github__/node_modules/.bin/flow check --show-all-errors
Error -------------------------------------------------------------------- flow-types/nonfb/libdef/babelTypes.js:1806:18

boolean [1] is not supported by unclassified use SubstOnPredT.

   flow-types/nonfb/libdef/babelTypes.js:1806:18
   1806|   declare export function isArrayExpression(node: ?Object, opts?: ?Object): boolean %checks (node instanceof BabelNodeArrayExpression)
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

References:
   flow-types/nonfb/libdef/babelTypes.js:1806:94
   1806|   declare export function isArrayExpression(node: ?Object, opts?: ?Object): boolean %checks (node instanceof BabelNodeArrayExpression)
                                                                                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [1]

Error -------------------------------------------------------------------- flow-types/nonfb/libdef/babelTypes.js:1807:18

boolean [1] is not supported by unclassified use SubstOnPredT.

   flow-types/nonfb/libdef/babelTypes.js:1807:18
   1807|   declare export function isAssignmentExpression(node: ?Object, opts?: ?Object): boolean %checks (node instanceof BabelNodeAssignmentExpression)
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

References:
   flow-types/nonfb/libdef/babelTypes.js:1807:99
   1807|   declare export function isAssignmentExpression(node: ?Object, opts?: ?Object): boolean %checks (node instanceof BabelNodeAssignmentExpression)
                                                                                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [1]

...

Found 238 errors
error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
```

Repro steps:

```
yarn
yarn clean-test
# run yarn flow:check or flow:watch to run Flow checks again afterwards
```

Issue related to: babel/babel#11671

Differential Revision: D22032614

fbshipit-source-id: d6c6663cd8e7a812759ef1b5ce7a80324bb1ba44
@kayhadrin
Copy link
Copy Markdown
Author

I tried import typeof * as BabelTypes from "@babel/types".
It seems like a good idea but this produces new Flow errors that don't normally appear when using import typeof BabelTypes from '@babel/types'.

E.g. from fbt project: facebook/fbt#150

$ /oss-fbt/__github__/node_modules/.bin/flow check --show-all-errors
Error -------------------------------------------------------------------- flow-types/nonfb/libdef/babelTypes.js:1806:18

boolean [1] is not supported by unclassified use SubstOnPredT.

   flow-types/nonfb/libdef/babelTypes.js:1806:18
   1806|   declare export function isArrayExpression(node: ?Object, opts?: ?Object): boolean %checks (node instanceof BabelNodeArrayExpression)
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

References:
   flow-types/nonfb/libdef/babelTypes.js:1806:94
   1806|   declare export function isArrayExpression(node: ?Object, opts?: ?Object): boolean %checks (node instanceof BabelNodeArrayExpression)
                                                                                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [1]


Error -------------------------------------------------------------------- flow-types/nonfb/libdef/babelTypes.js:1807:18

boolean [1] is not supported by unclassified use SubstOnPredT.

   flow-types/nonfb/libdef/babelTypes.js:1807:18
   1807|   declare export function isAssignmentExpression(node: ?Object, opts?: ?Object): boolean %checks (node instanceof BabelNodeAssignmentExpression)
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

References:
   flow-types/nonfb/libdef/babelTypes.js:1807:99
   1807|   declare export function isAssignmentExpression(node: ?Object, opts?: ?Object): boolean %checks (node instanceof BabelNodeAssignmentExpression)
                                                                                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [1]

...

Found 238 errors
error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

There seem to be a problem using %checks(...) in that particular scenario. :-(

I've asked the Flow team for help; in the meantime, do you reckon the current solution in this PR is good enough for now?

@kayhadrin
Copy link
Copy Markdown
Author

Ping @nicolo-ribaudo

The Flow team will fix the type issue mentioned above.
If we care about backward compatibility, we could add the declare export default {...} block; otherwise, people could use import typeof * as ... only if they update their Flow engine too.

What do you think about the rest of this PR?

@kayhadrin
Copy link
Copy Markdown
Author

🏓

@JLHwung JLHwung changed the base branch from master to main June 25, 2020 15:15
@JLHwung JLHwung self-requested a review June 25, 2020 15:15
@JLHwung
Copy link
Copy Markdown
Contributor

JLHwung commented Jun 25, 2020

@kayhadrin Can you rebase on main?

@kayhadrin kayhadrin force-pushed the babel/types/fix/export-flow-types branch from ee4bb1c to aade6dd Compare June 26, 2020 22:43
@kayhadrin
Copy link
Copy Markdown
Author

@kayhadrin Can you rebase on main?

Done.
But note that I found some lint errors due to someone else's changes after rebasing on master.

####
# make test
####
...
error: Delete `·` (prettier/prettier) at babel.config.js:3:26:
  1 | "use strict";
  2 | 
> 3 | module.exports = function (api) {
    |                          ^
  4 |   const env = api.env();
  5 | 
  6 |   const includeCoverage = process.env.BABEL_COVERAGE === "true";

@JLHwung
Copy link
Copy Markdown
Contributor

JLHwung commented Jun 30, 2020

@kayhadrin Please run make bootstrap after your rebased. It is due to prettier was updated recently but the one in your n_m is not sync with package locks.

@kayhadrin kayhadrin force-pushed the babel/types/fix/export-flow-types branch from aade6dd to b153d07 Compare July 1, 2020 06:38
@kayhadrin
Copy link
Copy Markdown
Author

Ok, rebased again on the main branch and rerun tests:

 PASS  packages/babel-types/test/misc.js
 PASS  packages/babel-types/test/builders/flow/declareClass.js
 PASS  packages/babel-types/test/cloning.js
 PASS  packages/babel-types/test/retrievers.js
 PASS  packages/babel-types/test/builders/flow/createTypeAnnotationBasedOnTypeof.js
 PASS  packages/babel-types/test/builders/es2015/templateElement.js
 PASS  packages/babel-types/test/builders/typescript/tsTypeParameter.js
 PASS  packages/babel-types/test/validators.js
 PASS  packages/babel-types/test/builders/experimental/classProperty.js
 PASS  packages/babel-types/test/regressions.js
 PASS  packages/babel-types/test/converters.js
 PASS  packages/babel-types/test/asserts.js

Test Suites: 12 passed, 12 total
Tests:       412 passed, 412 total
Snapshots:   21 passed, 21 total
Time:        5.063s
Ran all test suites matching /(packages|codemods|eslint)\/.*babel-types.*\/test/i.

@kayhadrin
Copy link
Copy Markdown
Author

@JLHwung 🏓

Copy link
Copy Markdown
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.

@JLHwung JLHwung added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Jul 7, 2020
nicolo-ribaudo
nicolo-ribaudo previously approved these changes Jul 7, 2020
@nicolo-ribaudo nicolo-ribaudo dismissed their stale review July 7, 2020 22:45

(sorry, I didn't mean to approve yet)

@nicolo-ribaudo
Copy link
Copy Markdown
Member

Just to understand, the declare export default {} bit is needed for backward compatibility, because it used to work with previous Babel versions? If so, I would love for a pr targeting the next-8-dev branch (after that this PR is merged to main and to next-8-dev) to remove it.

@kayhadrin
Copy link
Copy Markdown
Author

Just to understand, the declare export default {} bit is needed for backward compatibility, because it used to work with previous Babel versions?

Yes, it used to work but some other Flow type changes caused it to break (e.g. the addition of Node type references). Also, the usage of the ES6 export pattern instead of CommonJS also exposed a bug in the Flow engine, that was recently patched in 0.128.0 (see facebook/flow@d326475)

If so, I would love for a pr targeting the next-8-dev branch (after that this PR is merged to main and to next-8-dev) to remove it.

TBH, unless we're prepared to patch older Babel releases, I think I could already remove the declare export default {} part.
Instead, we should consider adding a few explanations in the README to show people how to import types from this file.
E.g. How to use import typeof * as BabelTypes from "@babel/types" since it was new to me before I submitted this PR.

@kayhadrin
Copy link
Copy Markdown
Author

@nicolo-ribaudo What would you like me to do then?
I'm a bit confused for now.

@liuxingbaoyu
Copy link
Copy Markdown
Member

Thank you for your PR! Since it is now obsolete, I will close it.

@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 Feb 6, 2024
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Feb 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area: flow outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: types PR: Bug Fix 🐛 A type of pull request used for our changelog categories

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants