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

chore(babel-plugin-remove-graphql-queries): Convert index.js #23954

Conversation

zachdtaylor
Copy link
Contributor

Description

Convert packages/babel-plugin-remove-graphql-queries/src/index.js to TypeScript

Related Issues

Related to #21995

@zachdtaylor zachdtaylor requested a review from a team as a code owner May 10, 2020 05:37
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label May 10, 2020
@ascorbic ascorbic added status: needs core review Currently awaiting review from Core team member and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels May 11, 2020
@pieh pieh self-assigned this May 18, 2020
return {
visitor: {
Program(path, state) {
Program(path: NodePath<Program>, state: any): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we type state to avoid any? AFAIK we don't pass any options so generic one should work (if there is one)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's hard to tell what state is supposed to be because there aren't many resources that I've found on writing babel plugins. Based on this type file, state is either the generic type S or any, and after some digging it looks like S is set to {} by default. Are you thinking we should just create our own type for state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update on this. I created a custom type for state called IState and I get an error:

Type '(this: {}, path: NodePath<Program>, state: IState) => void' is not assignable to type 'VisitNodeFunction<{}, Program> | VisitNodeObject<{}, Program> | undefined'.
  Type '(this: {}, path: NodePath<Program>, state: IState) => void' is not assignable to type 'VisitNodeFunction<{}, Program>'.
    Types of parameters 'state' and 'state' are incompatible.
      Property 'file' is missing in type '{}' but required in type 'IState'.

I think it's because the type of state is expected to be {}. However, when I explicitly declare the type like this:

Program(path: NodePath<Program>, state: {}): void {
  ...
}

I get an error saying property 'file' does not exist on type '{}'. I'm unsure how to resolve this issue at the moment. If you know a solution, @pieh, feel free to just add it in.

@pvdz pvdz added bot: merge on green Gatsbot will merge these PRs automatically when all tests passes topic: webpack/babel Webpack or babel and removed status: needs core review Currently awaiting review from Core team member labels Aug 5, 2020
@pvdz
Copy link
Contributor

pvdz commented Aug 5, 2020

Ok well the reported errors seem legit errors. So please run yarn typecheck locally and for anything that pops up :)

@pvdz pvdz removed the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Aug 5, 2020
@pvdz
Copy link
Contributor

pvdz commented Aug 11, 2020

@zachtylr21 friendly ping. Do you want to keep working on this? I think it was close to being merged?

@zachdtaylor
Copy link
Contributor Author

@pvdz Yes I'd like to finish this and get it merged. Apologies, I didn't see your comment from 6 days ago. Is running yarn typecheck the last thing to do?

@pvdz
Copy link
Contributor

pvdz commented Aug 11, 2020

I usually do yarn typecheck and yarn lint (and watch for errors) because for some reason sometimes errors sometimes only get caught by one or the other. That may have been fixed by now but that's what I still do :)

@zachdtaylor
Copy link
Contributor Author

@pvdz I'm getting an error that says Cannot find module 'gatsby'. Do I need to import gatsby in a different way?

@zachdtaylor
Copy link
Contributor Author

@pvdz There are also a bunch of linting errors in the gatsby-admin package, which is outside the scope of this PR.

@pvdz
Copy link
Contributor

pvdz commented Aug 11, 2020

The gatsby-admin stuff is either brand new or old problems that require a rebase/merge of master to resolve. I'll try that.

@pvdz
Copy link
Contributor

pvdz commented Aug 11, 2020

Some yarn typecheck stuff left it seems
image

@zachdtaylor
Copy link
Contributor Author

I'm a little confused at how to alter the graphql type to include those functions. Here is the definition of the graphql type, from gatsby/index.d.ts.

Screen Shot 2020-08-11 at 10 26 21 PM

It seems I need to alter the file gatsby/index.d.ts. Is that outside the scope of this PR?

Also, I'm not getting the errors on lines 249, 300, and 360. .unshiftContainer is defined in @types/babel__traverse/index.d.ts and path.node.callee.property is type any according to @babel/types/lib/index.d.ts, so path.node.callee.property.name should be valid.

Co-authored-by: Vladimir Razuvaev <vladimir.razuvaev@gmail.com>
@zachdtaylor
Copy link
Contributor Author

You guys will have to excuse my ignorance on a couple of these things, I'm pretty new to contributing to Gatsby.

After changing import { graphql } from "gatsby" to import graphql from "gatsby/graphql" like @vladar suggested, I'm getting a new error.

Screen Shot 2020-08-13 at 8 14 18 PM

I thought this meant I have to do npm install in the packages/gatsby directory, but doing that gave me more errors:

Screen Shot 2020-08-13 at 8 19 44 PM

I'm super confused as to why none of these packages can be found. Is there something I'm missing here? I really want to get this PR moving along but I keep running into frustrating issues like this.

@vladar
Copy link
Contributor

vladar commented Aug 14, 2020

I pulled your branch, ran yarn bootstrap in the monorepo root and I don't see this problem. I see several other errors though:

packages/babel-plugin-remove-graphql-queries/src/index.ts:249:33 - error TS2339: Property 'name' does not exist on type 'ArrayExpression | ArrowFunctionExpression | AssignmentExpression | AwaitExpression | ... 40 more ... | PrivateName'.
  Property 'name' does not exist on type 'ArrayExpression'.

249       path.node.callee.property.name === `useStaticQuery` &&
                                    ~~~~

packages/babel-plugin-remove-graphql-queries/src/index.ts:300:20 - error TS2339: Property 'unshiftContainer' does not exist on type 'NodePath<Program>'.

300               path.unshiftContainer(`body`, importDeclaration)
                       ~~~~~~~~~~~~~~~~

packages/babel-plugin-remove-graphql-queries/src/index.ts:360:20 - error TS2339: Property 'unshiftContainer' does not exist on type 'NodePath<Program>'.

360               path.unshiftContainer(`body`, importDeclaration)
                       ~~~~~~~~~~~~~~~~


Found 3 errors.

CI fails with exact same error.

So my best guess is that your dependency setup is messed up. Are you using npm? This may be the issue. If so I suggest you remove node_modules, run yarn in the monorepo root and try again.

@zachdtaylor
Copy link
Contributor Author

Ah, I was using npm. 🤦 I switched to yarn and now I'm seeing the same errors. I fixed them, and just pushed. I should note that the method unshiftContainer doesn't exist in any of Babel's type declaration files, so I had to change that method call to something that I believe is equivalent based on this StackOverflow post.

@zachdtaylor zachdtaylor requested a review from vladar August 15, 2020 03:21
@pvdz
Copy link
Contributor

pvdz commented Sep 8, 2020

It seems something snapshotty is failing over this thing fc97669#diff-0cee23a635d1c08646f35342b6dee83f

Copy link
Contributor

@pvdz pvdz left a comment

Choose a reason for hiding this comment

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

lgtm and tests are passing. Thank you 💜

@pvdz pvdz merged commit ef6f90c into gatsbyjs:master Sep 8, 2020
pragmaticpat pushed a commit to pragmaticpat/gatsby that referenced this pull request Apr 28, 2022
…s#23954)

* Declare Error class variables

* Apply linter

* Assert non-null NODE_ENV; remove unused state parameter

* Fix eslint warning about non-null assertion

* Declare return types on some functions

* Prefix GraphQLTag interface with I

* Declare function return types

* Change getTagImport return type to any

* Add babel types to dev dependencies

* Add types in all functions before default export

* Add types in all functions before default export

* Type function parameters

* Type jsx nodes

* Type jsx nodes

* Create visitor interfaces

* Move 'as NodePath'

* Use type instead of interface

* Use type

* Create custom State type

* Create custom State type

* Allow undefined members in StringInterpolationNotAllowedError

* Wrap if statement in block

Co-authored-by: Peter van der Zee <209817+pvdz@users.noreply.github.com>

* chore: format

* Change the way graphql is imported

Co-authored-by: Vladimir Razuvaev <vladimir.razuvaev@gmail.com>

* Fix typecheck errors

* chore: format

Co-authored-by: Peter van der Zee <209817+pvdz@users.noreply.github.com>
Co-authored-by: gatsbybot <mathews.kyle+gatsbybot@gmail.com>
Co-authored-by: Vladimir Razuvaev <vladimir.razuvaev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: webpack/babel Webpack or babel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants