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

Run "yarn rw-gen" during create redwood app #2770

Merged
merged 6 commits into from
Jun 10, 2021
Merged

Run "yarn rw-gen" during create redwood app #2770

merged 6 commits into from
Jun 10, 2021

Conversation

jtoar
Copy link
Contributor

@jtoar jtoar commented Jun 8, 2021

TypeScript Redwood Apps (created with yarn create redwood-app --ts) can't find the NotFoundPage in the Routes.tsx file because the types haven't been generated yet (#2754). This PR fixes this by generating the types as a part of the install.

I think the types are supposed to help JS projects too, so I made it so that they're generated for JS projects as well on install.

@jtoar jtoar self-assigned this Jun 8, 2021
@jtoar jtoar added the topic/crwa create-redwood-app label Jun 8, 2021
@thedavidprice thedavidprice added this to the next-release-priority milestone Jun 8, 2021
@jtoar jtoar linked an issue Jun 8, 2021 that may be closed by this pull request
@jtoar jtoar added the bug/confirmed We have confirmed this is a bug label Jun 8, 2021
@thedavidprice
Copy link
Contributor

One area where our create-redwood-app.js script fails is that we assume yarn install is always run. But we provide an option for --no-yarn-install, which, if used alone, will cause the script to fail because it tries to convert to TS using yarn rw ts-to-js. (One simple way to improve this is to set the condition to only run ts-to-js command if --yarn-install=true. I think a better solution would be to include the TS to JS transformation as a step in the installation script so that it could run whether or not yarn is installed.)

I'm curious if we'll have the same issue here where --no-yarn-install is passed but then the script fails because it can't generate the types... possibly?

@peterp
Copy link
Contributor

peterp commented Jun 9, 2021

Nice! Maybe we can add skip to the listr tasks if the install flag isn't present?

@jtoar
Copy link
Contributor Author

jtoar commented Jun 9, 2021

@thedavidprice linking #1954 just cause you mentioned that there too, and it does indeed fail if yarn install isn't true which is definitely not ideal.

I think a better solution would be to include the TS to JS transformation as a step in the installation script so that it could run whether or not yarn is installed

^ I'll see if I can include this here

@peterp
Copy link
Contributor

peterp commented Jun 9, 2021

@jtoar Could you try the ts-stuff in a different PR? I would like to get this one in alongside the eslint PR.

@jtoar
Copy link
Contributor Author

jtoar commented Jun 9, 2021

@peterp Absolutely; I can go ahead and rebase this one it if it looks ready

@jtoar
Copy link
Contributor Author

jtoar commented Jun 9, 2021

I included a check for --yarn-install just for now, so that it generates a TS project if --yarn-install=false (instead of failing). I did it via Listr's enabled property, but I could also do it via skip and provide a message:

enabled: () => typescript === false,
skip: () => {
  if (yarnInstall === false) {
    return 'skipped; needs ts-to-js binary'
  }
},

@jtoar jtoar requested a review from peterp June 9, 2021 19:01
Copy link
Contributor

@peterp peterp left a comment

Choose a reason for hiding this comment

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

Woot!

Copy link
Collaborator

@dac09 dac09 left a comment

Choose a reason for hiding this comment

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

LGTM too!

@jtoar jtoar merged commit bb5002c into redwoodjs:main Jun 10, 2021
@jtoar jtoar deleted the ds-run-rw-gen-after-install branch June 10, 2021 15:15
thedavidprice pushed a commit that referenced this pull request Jun 10, 2021
* run rw-gen after ts-to-js

* skip if install is skipped

* skip instead of disable

* generate ts project if yarn install is false

Co-authored-by: Daniel Choudhury <dannychoudhury@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/confirmed We have confirmed this is a bug topic/crwa create-redwood-app
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New TS project can't find NotFoundPage
5 participants