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): convert babel-loader-helpers to typescript #36237

Merged

Conversation

Kornil
Copy link
Contributor

@Kornil Kornil commented Jul 26, 2022

Description

Convert babel-loader-helpers to TS.

Documentation

Related Issues

Previous PR was forgotten and recently closed, as suggested in the closing message, I updated the code and re-opened it. #25100

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jul 26, 2022
@imjoshin imjoshin added topic: TypeScript Issues and PRs related to TS in general, public typings or gatsby-plugin-typescript and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Jul 28, 2022
Having trouble merging master in, so just making a quick change to try to trigger builds. 😄
@imjoshin imjoshin dismissed a stale review via d377a79 July 29, 2022 15:13
@Kornil
Copy link
Contributor Author

Kornil commented Jul 30, 2022

Hi @j0sh77, I've seen the e2e tests failing so I re-merged the master branch, but I see the tests not starting again. Should I just re-cherry-pick your commits to kick off the build?
I'm not sure how to proceed from here.

@imjoshin
Copy link
Contributor

imjoshin commented Aug 1, 2022

Hi @j0sh77, I've seen the e2e tests failing so I re-merged the master branch, but I see the tests not starting again. Should I just re-cherry-pick your commits to kick off the build? I'm not sure how to proceed from here.

Yeah, there seems to be an issue with builds kicking off commits from forked repos?

The production runtime e2e tests are flaky, and the react 18 ones are perpetual failures at this point. We're hoping to get those addressed soon, but you can ignore them for the time being.

Again, thank you so much for this PR and taking the time to rebase. 💜

@Kornil
Copy link
Contributor Author

Kornil commented Aug 1, 2022

Thank you for your answer, that explained a lot!

I see now only one react_18 test is failing (remote-file.js).
I've checked it but does not seem to have any relation to these changes so I'll just assume it's a flake.

pluginBabelConfig.stages[stage].plugins.forEach(plugin => {
reduxPlugins.push(
babel.createConfigItem([resolve(plugin.name), plugin.options], {
dirname: plugin.name,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think adding the dirname is ok here, but should we keep the name key as well?

https://github.com/babel/babel/blob/main/packages/babel-core/src/config/item.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was going to leave it but the types from babel changed and no longer include it.
The createConfigItem method that we use here comes from this type https://github.com/babel/babel/blob/main/packages/babel-core/src/config/item.ts#L28 and it now accepts only dirname and type.

Copy link
Contributor

@LekoArts LekoArts Aug 3, 2022

Choose a reason for hiding this comment

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

Looking at the current babel docs and previous versions the createConfigItem now has these parameters so changing them feels correct (and not adding invalid name key)

Copy link
Contributor

@imjoshin imjoshin left a comment

Choose a reason for hiding this comment

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

Thanks again for your patience, understanding, and contribution to Gatsby! 💜

@imjoshin imjoshin merged commit ee2b0aa into gatsbyjs:master Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: TypeScript Issues and PRs related to TS in general, public typings or gatsby-plugin-typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants