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 #25100

Closed

Conversation

Kornil
Copy link
Contributor

@Kornil Kornil commented Jun 18, 2020

Convert babel-loader-helpers to TS.

On line 101 and 109, I get a type error that I am not able to fix related to @babel/core types not allowing createConfigItem to have a name key in its payload.

I'm not sure why tests are failing, other than the imports, the types shouldn't have affected prod. Any insights on what is causing it? Switching back the babel-loader imports doesn't do anything.

Related Issues

#21995

@Kornil Kornil requested a review from a team as a code owner June 18, 2020 15:15
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jun 18, 2020
@ascorbic ascorbic removed the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jun 18, 2020
Copy link
Contributor

@ascorbic ascorbic left a comment

Choose a reason for hiding this comment

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

Thanks for this. The typecheck tests are failing for this, largely because you'll need to update the babel tests to cast the Mock types. Make sure you run yarn typecheck to ensure it's all working.

@@ -16,13 +29,23 @@ const loadCachedConfig = () => {
return pluginBabelConfig
}

const getCustomOptions = stage => {
export const getCustomOptions = (stage: string): IBabelStage["options"] => {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a string enum Stage which should be used for the argument here. You'll find it in commands/types.ts

const index = _.findIndex(
items,
i => i.file.resolved === itemToMerge.file.resolved
i => i.file!.resolved === itemToMerge.file!.resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use optional chaining rather than not-null assertion here and elsewhere, so avoid potential runtime errors

@Kornil
Copy link
Contributor Author

Kornil commented Jun 18, 2020

Thank you @ascorbic for the review. I updated the test types, the snapshots, and used the Stage enum instead of the string I had before. I also took the opportunity to clean up the babelrc reducer types since they had a duplicate stage key (introduced by me in a previous PR).

I added a 👍 on your comments to indicate I solved the issues. If you feel the feedback was addressed properly, feel free to resolve the conversations!

pluginBabelConfig.stages[stage].plugins.forEach(plugin => {
const reduxPlugins: PluginItem[] = []
const reduxPresets: PluginItem[] = []
pluginBabelConfig.stages[stage!].plugins.forEach(plugin => {
reduxPlugins.push(
babel.createConfigItem([resolve(plugin.name), plugin.options], {
name: 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.

Looks like this is an error. It seems it's correct that name isn't a valid key. It accepts just dirname and type: https://babeljs.io/docs/en/babel-core#createconfigitem

Copy link
Contributor Author

@Kornil Kornil Jun 19, 2020

Choose a reason for hiding this comment

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

What do you think would be the best option here? I'm not sure how it will affect prod to remove the name key.

Copy link
Contributor

@ascorbic ascorbic left a comment

Choose a reason for hiding this comment

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

Just a few nits, though we'll need to check what's goign on with the name prop, which seems to be an error.

const pluginBabelConfig = loadCachedConfig()
return pluginBabelConfig.stages[stage].options
}

const prepareOptions = (babel, options = {}, resolve = require.resolve) => {
let pluginBabelConfig = loadCachedConfig()
type prepareOptionsType = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than typing the whole function like this, can you just type the options object?

pluginBabelConfig.stages[stage].plugins.forEach(plugin => {
const reduxPlugins: PluginItem[] = []
const reduxPresets: PluginItem[] = []
pluginBabelConfig.stages[stage!].plugins.forEach(plugin => {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you've typed the options, and the stage key as optional you'll need a runtime null check for stage rather than this assertion.

reduxPlugins.push(
babel.createConfigItem([resolve(plugin.name), plugin.options], {
name: plugin.name,
type: `plugin`,
})
)
})
pluginBabelConfig.stages[stage].presets.forEach(preset => {
pluginBabelConfig.stages[stage!].presets.forEach(preset => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same. If you've done a runtime check then this will have been narrowed to exclude null.

@@ -98,17 +122,34 @@ const prepareOptions = (babel, options = {}, resolve = require.resolve) => {
]
}

const mergeConfigItemOptions = ({ items, itemToMerge, type, babel }) => {
type mergeConfigItemOptionsType = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

As above, rather than typing the whole function like this, type the props object.

@Kornil
Copy link
Contributor Author

Kornil commented Jun 19, 2020

Addressed the second wave of feedback, let me know your opinion about the name issue on line 98 and 106.

@Kornil Kornil requested a review from ascorbic June 19, 2020 17:48
@LekoArts LekoArts added topic: TypeScript Issues and PRs related to TS in general, public typings or gatsby-plugin-typescript and removed topic: TypeScript migration labels May 28, 2021
@imjoshin
Copy link
Contributor

Hey! Thanks so much for opening this pull request!

Sadly this PR got stale and didn't have any activity for some time. We're trying to do better with PR reviews! To get a better overview of all actionable PRs we're going through all open PRs and triage them. Since we won't be able to do everything and adding new features always means added maintenance burden, we have to be more picky about what's beneficial for the average user and the project itself longterm.

We think this is a great PR and would love to see it land in Gatsby. We're closing this PR for now but if you're able to rebase onto the latest master branch and try it out with the latest next versions, we'd be happy to review the PR when its re-opened.

We absolutely want to have you as a contributor and are sorry for any inconveniences we caused with replying too late to this PR.

Thanks for submitting to Gatsby! 💜

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.

4 participants