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(gatsby) Rewrite requires-writer.js to TypeScript #24289

Merged
merged 1 commit into from
May 26, 2020
Merged

chore(gatsby) Rewrite requires-writer.js to TypeScript #24289

merged 1 commit into from
May 26, 2020

Conversation

alexpyzhianov
Copy link
Contributor

Description

This adds type annotations to requires-writer.js from the gatsby package

I had to introduce one assertion for matchPath !== undefined as IGatsbyPage it's of type string | undefined, but everything in requires-writer.js assumes that it's a string. It would throw an error anyway, I just made a bit more pretty with reporter.panic()

NOTE:
requires-writer.js uses the reporter from gatsby-cli, and not the TS source, but the compiled version. Not sure if it's ok. Maybe we should extract reporter into a separate package if it is used in both gatsby and gasby-cli

Documentation

No changes required

Related Issues

Related to #21995

- Mostly just adds TypeScript type annotations

- Introduces one assertion for matchPath !== undefined
as IGatsbyPage has it as string | undefined, but everything
in requires-writer.js assumes that it's a string. It would throw an
error anyway, I just made a bit more pretty with reporter.panic()

WARNING:
- requires-writer uses reporter from gatsby-cli, and not the
TS source, but the compiled version. Not sure if it's ok.
Maybe we should extract reporter into a separate package if
it is used in both gatsby and gasby-cli
@alexpyzhianov alexpyzhianov requested a review from a team as a code owner May 21, 2020 07:11
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label May 21, 2020
@alexpyzhianov alexpyzhianov marked this pull request as draft May 21, 2020 07:11
@alexpyzhianov alexpyzhianov marked this pull request as ready for review May 21, 2020 07:39
@freiksenet freiksenet removed the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label May 22, 2020
Copy link
Contributor

@blainekasten blainekasten left a comment

Choose a reason for hiding this comment

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

This looks great to me, let's merge it!

Thank you so much for contributing to our TypeScript refactor! We have more work to do and we would love to have you stay involved in our transition. Please submit more PRs! 💜

@blainekasten
Copy link
Contributor

I had to introduce one assertion for matchPath !== undefined as IGatsbyPage it's of type string | undefined, but everything in requires-writer.js assumes that it's a string. It would throw an error anyway, I just made a bit more pretty with reporter.panic()

This was the perfect thing to do :)

NOTE:
requires-writer.js uses the reporter from gatsby-cli, and not the TS source, but the compiled version. Not sure if it's ok. Maybe we should extract reporter into a separate package if it is used in both gatsby and gasby-cli

I agree with this, it's been something on our mind. We'll get there eventually :D

Thanks for the awesome PR!

@blainekasten blainekasten merged commit 08709a2 into gatsbyjs:master May 26, 2020
johno pushed a commit to johno/gatsby that referenced this pull request May 28, 2020
- Mostly just adds TypeScript type annotations

- Introduces one assertion for matchPath !== undefined
as IGatsbyPage has it as string | undefined, but everything
in requires-writer.js assumes that it's a string. It would throw an
error anyway, I just made a bit more pretty with reporter.panic()

WARNING:
- requires-writer uses reporter from gatsby-cli, and not the
TS source, but the compiled version. Not sure if it's ok.
Maybe we should extract reporter into a separate package if
it is used in both gatsby and gasby-cli
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants