feat: Add logging action and middleware

[stefanprobst]

This is an implementation proposal for the Structured logging RFC, based on @m-allanson 's work there.

It adds:

• SET_LOGGER action: for setting the logger instance. currently this just takes the reporter from the cli. The only place where the reporter is still directly imported is in bootstrap/index (and in the commands which haven't been touched).
• LOG_MESSAGE action: to log a message
• middleware to forward log messages to the logger. If we wanted, this would be a good place to add a timestamp, or additionally forward other actions (and their payload) to the logger.

[stefanprobst]

Does anyone know why the snapshot fails on Windows only?

- Snapshot
+ Received

- "The plugin "test" must set the absolute path to the page component when creating a page."
+ "The plugin "test" must set the absolute path to the page component when creating a page."

[wardpeet]

This looks nice! I actually like it. We probably want to make SET_LOGGER an array which people can hook into so we can have multiple sources like file logger & screen logger but for now this is great (YAGNI).

I wonder if we should add "telemetry" to redux as well, instead of adding calls inside our code we could just add a redux middleware that watch events. We could add some metadata to redux so telemtry has some extra data.

I ran it locally on windows and all tests passes so unsure what azure does differently.

$ jest packages/gatsby/src/redux/__tests__/pages.js
 PASS  packages/gatsby/src/redux/__tests__/pages.js (9.523s)
  Add pages
    √ allows you to add pages (121ms)
    √ Fails if path is missing (3ms)
    √ Fails if component path is missing (3ms)
    √ Fails if the component path isn't absolute (3ms)
    √ Fails if use a reserved field in the context object (4ms)
    √ adds an initial forward slash if the user doesn't (2ms)
    √ allows you to add pages with context (115ms)
    √ allows you to add pages with matchPath (98ms)
    √ allows you to add multiple pages (77ms)
    √ allows you to update existing pages (based on path) (42ms)
    √ allows you to delete paths (2ms)

Some bikeshedding:

const { store } = require(`../redux`)
const { actions } = require(`../redux/actions`)

const { dispatch } = store
const { log } = actions

I feel that we need some utitlities for this. I actualy liked boundActionCreators 😛. Perhaps it's me just being lazy but I would be happy with.

const { dispatch } = require(`../redux`)
const { actions } = require(`../redux/actions`)

const { log } = actions

[stefanprobst]

We probably want to make SET_LOGGER an array which people can hook into so we can have multiple sources like file logger & screen logger

Yes. Alternatively we could setLogger(new GatsbyLoggerBoss()), which handles distributing messages to file/screen etc. on its own.

I wonder if we should add "telemetry" to redux as well

I haven't really looked at the telemetry stuff but I'd imagine this would work quite well. I mean I even think the api-runner could work quite well as a middleware.

I actualy liked boundActionCreators

Why I like dispatching is because it enforces the mental model better. But yes there should be less to import. We wanted to reorganize actions anyway (better separate internal from public ones) -- we could just export them together with the store:

const { actions, store: { dispatch } } = require('../redux')
dispatch(actions.log('Hello world')
// actually shorter than
boundActionCreators.log('Hello world')

[DSchau]

@stefanprobst this is looking good!

Quick question - we'd like this functionality to be seamlessly integrated to even existing applications, and the easiest way to do that is by extending our reporter (e.g. reporter.warn, reporter.error, etc.) to integrate with e.g. our redux store.

Have you thought about a way we could make this functionality backwards compatible in these cases, and in doing so, require no changes for the end user/plugin author?

[stefanprobst]

@DSchau
There are currently two ways for plugin authors to use the reporter:

• import it directly from gatsby-cli/lib/reporter: this is bad practice, but will continue to work
• by using the reporter that gets passed down by the api runner: for this case we pass down a reporter object that proxies the reporter methods to the log action. (Currently this will then again be handled by the real reporter, since we set it to handle the log actions)

EDIT: What's currently missing is reporter.stripIndent and similar

[KyleAMathews]

yeah @stefanprobst we don't want to add new APIs for this.

[stefanprobst]

@KyleAMathews i don't understand, what new APIs?

[DSchau]

@stefanprobst after taking a deeper look at this--I understand what you're doing now!

reporter.warn, reporter.info, etc. all proxy to the log action, which is exactly what we want.

My main question is why are we swapping things to a dispatch instead of just invoking the same methods in some areas? For example, in this file why not just keep it report.panic?

[KyleAMathews]

What I mean is we want people to keep using the reporter in their plugins as well as us in core.

We want the reporter to add its own redux implemention for to store data and print out logs. Then core can send logs to it as needed.

This way we can do a gradual migration to more structured logging via overloading the first prop to the reporter to also accept an object for more detailed log entries.

[stefanprobst]

@KyleAMathews
TBH I'm a bit confused where this has been discussed? I'm ok with closing this, but to clarify what is proposed here and where I see the advantages:

• The main idea is to log messages by dispatching a redux action. Messages are handled by a redux middleware, which forwards the log actions to a Logger that is kept in the redux store as application state. The messages themselves are not kept in the store.
• Why even use redux for this?
  • Since we already use action dispatching in core (and might want to do so even more in the future), a middleware knows quite a lot about what is going on internally, and could forward this information to a Logger as well. The ability to not only log explicit log messages, but every action/event that flows through core, could be very useful. It's also a good place to add additional info like a timestamp to log events.
  • Some action creators do quite a bit of work, like input validation. When there is an error, like a validation failure, we currently have no good way (actually 4 different ways) to exit the action creator -- which should return some action. Having a log action would solve this, otherwise we have to keep doing things like dispatching a dummy VALIDATION_ERROR action.
• We need to still provide the reporter interface for plugins, since this is what gets passed to APIs by the api runner. This is why a reporter proxy is passed down by the api runner, which translates calls to reporter.error(message) etc. to dispatch(log({ type: 'error', message })). @DSchau In core, we can use action dispatching directly.

Question:

• Could you elaborate what the reporter having "its own redux implementation" would look like? And what it is for exactly? Is it to store the message history in redux?

[KyleAMathews]

My assumption is that we need to keep everything in gatsby-cli as there's plenty of plugins and places in core and I'm sure people's sites that use it for logging — we want to be sure all data is going through the same code. If we can successfully send everything through our core redux, that'd be great as it'd be simpler.

What I don't like is changing the API for passing in data — moving from reporter.error to dispatch(log({ type: 'error', message })) is a step or two backwards as far as API quality goes and also adds a lot of noise to this change.

Also not sure why action creators should have to return an action?

Another desired feature we need to account for is we'll be changing the CLI design to persistent errors/warnings at the bottom of the screen — this will mean that code that logs an error/warning will also need to be able to detect when that's fixed. This will probably be an opt-in feature but we can already do stuff like this like with e.g. xstate, we know if a component has a babel or graphql error now and when it's fixed and can use that to show/remove warnings

gatsby/packages/gatsby/src/redux/machines/page-component.js

Lines 25 to 26 in a93963b

	QUERY_EXTRACTION_GRAPHQL_ERROR: `queryExtractionGraphQLError`,
	QUERY_EXTRACTION_BABEL_ERROR: `queryExtractionBabelError`,

[stefanprobst]

@KyleAMathews

we need to keep everything in gatsby-cli as there's plenty of plugins and places in core [...] that use it for logging

I think for plugins it has always been bad practice to directly import from gatsby-cli instead of using the reporter passed down by the API runner. (Note however, that directly importing will continue to work, just bypass the middleware.)
In core we can freely change stuff, this is what the noise in this PR does. I think it is actually one of the advantages of the approach taken here that there is exactly one place where reporter is imported into core, and that we can change its interface without problems.

we want to be sure all data is going through the same code

I agree, which is why directly importing from gatsby-cli is bad.

a step or two backwards as far as API quality goes

It's hard to respond to this, except that I disagree. It's just standard redux, which imho is a good fit for us and we should use it much more consequently than we currently do.

Also not sure why action creators should have to return an action?

Action creators must create an action, because redux will throw if you try to dispatch something that is not an action (i.e. an object with a type property):

const { createStore } = require('redux')
const store = createStore(state => state)
const doSomething = () => 'Error' // throws
// const doSomething = () => ({ type: 'ERROR' }) // works
store.dispatch(doSomething())

we'll be changing the CLI design to persistent errors/warnings at the bottom of the screen — this will mean that code that logs an error/warning will also need to be able to detect when that's fixed

I like that. And I think it's actually a lot easier with a middleware approach because that is a place that already knows about every action flowing through core.

Btw imho in the future it would we good to have these kinds of discussions as part of the RFC process.

[KyleAMathews]

Btw imho in the future it would we good to have these kinds of discussions as part of the RFC process.

This is part of the RFC process :-P many RFCs need a POC PR put up to flesh out the ideas enough to discuss things deeply. E.g. @m-allanson's RFC has a lot of open questions at the end that we should resolve before the RFC is accepted. You've also changed the API here from what he wrote which, if we accept it, would need reflected there, etc.

Beyond changing APIs, I don't think we want middleware but to keep things in a reducer. We want to be able to compare old state to new state as to e.g. persist errors at the bottom, we'd need to avoid printing out a given error multiple times which would require knowing that the error is already being displayed.

[stefanprobst]

This is part of the RFC process

Ok cool, I feel like it hasn't been 100% clear to me how this is supposed to work. In any case we should ensure that discussions happening in issue comments stay discoverable from the RFC.

I don't think we want middleware but to keep things in a reducer ... to be able to compare old state to new state

Middleware and reducer are for different things -- middleware is for side-effect handling, and you have access to the whole store & can dispatch actions from there, e.g. to put things in the store with a reducer, which might make sense for the removing-notifications-case depending on how the new reporter would implement this exactly.

I think a logging middleware would mean very few constraints on how this can be evolved in the future, but we can discuss further in an alternative proposal.