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

Update json logger in gatsby-cli to TS #24140

Merged
merged 4 commits into from
May 19, 2020
Merged

Update json logger in gatsby-cli to TS #24140

merged 4 commits into from
May 19, 2020

Conversation

wiput1999
Copy link
Contributor

@wiput1999 wiput1999 commented May 16, 2020

Description

This PR migrates gatsby-cli/src/reporter/loggers/json/index.js to TypeScript.

Related Issues

Part of #21995.

@wiput1999 wiput1999 requested a review from a team as a code owner May 16, 2020 13:01
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label May 16, 2020
const sanitizedAction = sanitizeAction(action)

process.stdout.write(JSON.stringify(sanitizedAction) + `\n`)
})
Copy link
Contributor

Choose a reason for hiding this comment

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

We actually want to change this module to not invoke functionality on import. So this onLogAction should be wrapped in an exported function, which should then be executed in the parent module. Mind making these changes?

export function initializeJSONLogger() {
  onLogAction(....)
}

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've changed this to named export initializeJSONLogger

import stripAnsi from "strip-ansi"
import _ from "lodash"

const isStringPayload = (action: ActionsUnion): action is ISetStatus =>
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 get away without this action is ISetStatus ?

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 use action is ISetStatus to filter out ISetStatus which payload is a string.

action.payload.statusText = stripAnsi(action.payload.statusText)
}

return action
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 there is a functional change going on here. In the previous version, we were creating a new object and spreading values into it. This had the benefit that we weren't mutating the underlying object.

I think there is a risk here that this function in it's new state would actually be modifying values that are in redux state (thanks JS object referencing.)

Let's change this to create a new object instead of modifying the one passed in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I use guideline from IPC to copy new object before stripAnsi.

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.

Thank you for this and great start! Just needs a few tweaks

@blainekasten blainekasten added status: awaiting author response Additional information has been requested from the author and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels May 18, 2020
@blainekasten blainekasten self-assigned this May 18, 2020
@wiput1999 wiput1999 requested a review from blainekasten May 18, 2020 19:11
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 blainekasten added bot: merge on green Gatsbot will merge these PRs automatically when all tests passes and removed status: awaiting author response Additional information has been requested from the author labels May 19, 2020
@gatsbybot gatsbybot merged commit 16a8228 into gatsbyjs:master May 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants