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

feat(conditional-page-builds): track potentially unsafe Node.js builtin modules usage #29560

Merged
merged 5 commits into from
Feb 22, 2021

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Feb 18, 2021

Note: best to check changes with whitespace changes hidden as I did some try/catch wrapping which just changed indentation a lot

Description

Usage of certain Node.js builtin modules in SSR (primarly from gatsby-ssr, but not limited to it) risks that our tracking wether html file can be reused from previous build is not catching all the cases that ideally should case file to rebuild.

Current list of builtin modules that are "tracked":

    const builtinModulesToTrack = [
      `fs`,
      `http`,
      `http2`,
      `https`,
      `child_process`,
    ]

In those cases those modules are used we set a flag so that following builds would rebuild all the pages as we can't be certain of the exact "contract" usage of those APIs have. We also do display warnings about those (including code location) and that future builds will be full ones and not selective:
Screenshot 2021-02-18 at 01 12 07

Related Issues

[ch24532]

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Feb 18, 2021
@pieh pieh added topic: build Related to the Gatsby build process topic: SSG* and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Feb 18, 2021
@@ -0,0 +1,4 @@
/* eslint-disable filenames/match-regex */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is because filename (child_process.js doesn't match our eslint rule :( )

preBodyComponents = preBodyComponents.concat(sanitizeComponents(components))
}
// for this to work we need this function to be sync or at least ensure there is single execution of it at a time
global.unsafeBuiltinUsage = []
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This hurts my feelings, but I essentially need "context" that "unconnected" arbitrary code can access and push to.

@pieh pieh force-pushed the v3/webpack-5-ssr-fs-tracking branch 5 times, most recently from 5e71bcc to dedecfd Compare February 18, 2021 21:47
@pieh pieh marked this pull request as ready for review February 18, 2021 22:32
@pieh pieh force-pushed the v3/webpack-5-ssr-fs-tracking branch from dedecfd to e70af97 Compare February 18, 2021 22:39
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

I need to try it, some small nits and questions.

packages/gatsby/src/utils/webpack.config.js Outdated Show resolved Hide resolved
Comment on lines +422 to +425
return { html, unsafeBuiltinsUsage: global.unsafeBuiltinUsage }
} catch (e) {
e.unsafeBuiltinsUsage = global.unsafeBuiltinUsage
throw e
Copy link
Contributor

Choose a reason for hiding this comment

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

did anything change except this?

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 suggest checking with "hide whitespace changes" ( https://github.com/gatsbyjs/gatsby/pull/29560/files?w=1#diff-4cd5b620539129dfa79254b9cdae3bd9b619426b83b412d2647cfb416730d362 )

This whole file added

// for this to work we need this function to be sync or at least ensure there is single execution of it at a time
  global.unsafeBuiltinUsage = []

at the start of the function, changed return to return object instead of string and wrapped everything in try/catch to decorate caught error with unsafeBuiltinUsage to not "lose" it's usage (even tho probably this is might be not needed, it feels safer to do)

store.dispatch({
type: `HTML_GENERATED`,
payload: pageSegment,
type: `SSR_USED_UNSAFE_BUILTIN`,
Copy link
Contributor

Choose a reason for hiding this comment

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

How does someone clear this flag? Running gatsby clean and/or gatsby cleans cache?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this flag gets cleared when SSR compilation hash changes (as long as we use same SSR compilation hash is used - we know that if previous build used unsafe builtin methods - following builds will continue to do so, because it's the same code running).

Changing SSR compilation hash also cause all pages to be rebuild so we are "covered" for that case

packages/gatsby/src/commands/build-html.ts Outdated Show resolved Hide resolved
wardpeet
wardpeet previously approved these changes Feb 22, 2021
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

L

@pieh pieh force-pushed the v3/webpack-5-ssr-fs-tracking branch from 235419d to 197cb62 Compare February 22, 2021 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: build Related to the Gatsby build process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants