-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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): enable query on demand (and lazy images) by default for local development #28787
Merged
Merged
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
9d2738d
feat(flags): add LOCKED_IN mode for flags for cases where opt-in feat…
pieh 8f8af73
feat(gatsby): enable query on demand (and lazy images) by default for…
pieh ecef23b
fix "lint"
pieh ff4bf71
chore(telemetry): don't track usage of query on demand / lazy images …
pieh e22fc5e
use plain false instead of weird Symbol do opt out of telemetry tracking
pieh 4e76c5b
Update packages/gatsby/src/utils/handle-flags.ts
pieh 0308d2e
update snapshots
pieh File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -85,9 +85,7 @@ Object { | |
"umbrellaIssue": "test", | ||
}, | ||
], | ||
"message": " | ||
|
||
We're shipping new features! For final testing, we're rolling them out first to a small % of Gatsby users | ||
Comment on lines
-88
to
-90
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Result of adding checks before appending newlines to |
||
"message": "We're shipping new features! For final testing, we're rolling them out first to a small % of Gatsby users | ||
and your site was automatically chosen as one of them. With your help, we'll then release them to everyone in the next minor release. | ||
|
||
We greatly appreciate your help testing the change. Please report any feedback good or bad in the umbrella issue. If you do encounter problems, please disable the flag by setting it to false in your gatsby-config.js like: | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,3 @@ | ||
import sampleSiteForExperiment from "./sample-site-for-experiment" | ||
import _ from "lodash" | ||
import semver from "semver" | ||
|
||
|
@@ -29,14 +28,16 @@ export const satisfiesSemvers = ( | |
return result | ||
} | ||
|
||
export type fitnessEnum = true | false | "OPT_IN" | ||
export type fitnessEnum = true | false | "OPT_IN" | "LOCKED_IN" | ||
|
||
export const DO_NOT_TRACK_THIS_FLAG = Symbol(`DO_NOT_TRACK_THIS_FLAG`) | ||
|
||
export interface IFlag { | ||
name: string | ||
env: string | ||
description: string | ||
command: executingCommand | ||
telemetryId: string | ||
telemetryId: string | typeof DO_NOT_TRACK_THIS_FLAG | ||
pieh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// Heuristics for deciding if a flag is experimental: | ||
// - there are known bugs most people will encounter and that block being | ||
// able to use Gatsby normally | ||
|
@@ -47,13 +48,20 @@ export interface IFlag { | |
// resolved and ~50+ people have tested it, experimental should be set to | ||
// false. | ||
experimental: boolean | ||
// Generally just return true. | ||
// | ||
// False means it'll be disabled despite the user setting it true e.g. | ||
// it just won't work e.g. it doesn't have new enough version for something. | ||
// | ||
// OPT_IN means the gatsby will enable the flag (unless the user explicitly | ||
// disablees it. | ||
/** | ||
* True means conditions for the feature are met and can be opted in by user. | ||
* | ||
* False means it'll be disabled despite the user setting it true e.g. | ||
* it just won't work e.g. it doesn't have new enough version for something. | ||
* | ||
* OPT_IN means the gatsby will enable the flag (unless the user explicitly | ||
* disables it. | ||
* | ||
* LOCKED_IN means that feature is enabled always (unless `noCI` condition is met). | ||
* This is mostly to provide more meaningful terminal messages instead of removing | ||
* flag from the flag list when users has the flag set in configuration | ||
* (avoids showing unknown flag message and shows "no longer needed" message). | ||
*/ | ||
testFitness: (flag: IFlag) => fitnessEnum | ||
includedFlags?: Array<string> | ||
umbrellaIssue?: string | ||
|
@@ -70,8 +78,6 @@ const activeFlags: Array<IFlag> = [ | |
description: `Enable all experiments aimed at improving develop server start time`, | ||
includedFlags: [ | ||
`DEV_SSR`, | ||
`QUERY_ON_DEMAND`, | ||
`LAZY_IMAGES`, | ||
`PRESERVE_FILE_DOWNLOAD_CACHE`, | ||
`PRESERVE_WEBPACK_CACHE`, | ||
], | ||
|
@@ -91,70 +97,33 @@ const activeFlags: Array<IFlag> = [ | |
name: `QUERY_ON_DEMAND`, | ||
env: `GATSBY_EXPERIMENTAL_QUERY_ON_DEMAND`, | ||
command: `develop`, | ||
telemetryId: `QueryOnDemand`, | ||
telemetryId: DO_NOT_TRACK_THIS_FLAG, | ||
experimental: false, | ||
description: `Only run queries when needed instead of running all queries upfront. Speeds starting the develop server.`, | ||
umbrellaIssue: `https://gatsby.dev/query-on-demand-feedback`, | ||
noCI: true, | ||
testFitness: (): fitnessEnum => { | ||
// Take a 10% of slice of users. | ||
if (sampleSiteForExperiment(`QUERY_ON_DEMAND`, 10)) { | ||
let isPluginSharpNewEnoughOrNotInstalled = false | ||
try { | ||
// Try requiring plugin-sharp so we know if it's installed or not. | ||
// If it does, we also check if it's new enough. | ||
// eslint-disable-next-line | ||
require.resolve(`gatsby-plugin-sharp`) | ||
const semverConstraints = { | ||
// Because of this, this flag will never show up | ||
"gatsby-plugin-sharp": `>=2.10.0`, | ||
} | ||
if (satisfiesSemvers(semverConstraints)) { | ||
isPluginSharpNewEnoughOrNotInstalled = true | ||
} | ||
} catch (e) { | ||
if (e.code === `MODULE_NOT_FOUND`) { | ||
isPluginSharpNewEnoughOrNotInstalled = true | ||
} | ||
} | ||
|
||
if (isPluginSharpNewEnoughOrNotInstalled) { | ||
return `OPT_IN` | ||
} else { | ||
// Don't opt them in but they can still manually enable. | ||
return true | ||
} | ||
} else { | ||
// Don't opt them in but they can still manually enable. | ||
return true | ||
} | ||
}, | ||
testFitness: (): fitnessEnum => `LOCKED_IN`, | ||
}, | ||
{ | ||
name: `LAZY_IMAGES`, | ||
env: `GATSBY_EXPERIMENTAL_LAZY_IMAGES`, | ||
command: `develop`, | ||
telemetryId: `LazyImageProcessing`, | ||
telemetryId: DO_NOT_TRACK_THIS_FLAG, | ||
experimental: false, | ||
description: `Don't process images during development until they're requested from the browser. Speeds starting the develop server. Requires [email protected] or above.`, | ||
umbrellaIssue: `https://gatsby.dev/lazy-images-feedback`, | ||
noCI: true, | ||
testFitness: (): fitnessEnum => { | ||
// Take a 10% of slice of users. | ||
if (sampleSiteForExperiment(`QUERY_ON_DEMAND`, 10)) { | ||
const semverConstraints = { | ||
// Because of this, this flag will never show up | ||
"gatsby-plugin-sharp": `>=2.10.0`, | ||
} | ||
if (satisfiesSemvers(semverConstraints)) { | ||
return `OPT_IN` | ||
} else { | ||
// gatsby-plugi-sharp is either not installed or not new enough so | ||
// just disable — it won't work anyways. | ||
return false | ||
} | ||
const semverConstraints = { | ||
// Because of this, this flag will never show up | ||
"gatsby-plugin-sharp": `>=2.10.0`, | ||
} | ||
if (satisfiesSemvers(semverConstraints)) { | ||
return `LOCKED_IN` | ||
} else { | ||
return true | ||
// gatsby-plugin-sharp is either not installed or not new enough so | ||
// just disable — it won't work anyways. | ||
return false | ||
} | ||
}, | ||
}, | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just "random" thing I spotted when looking for qod related changes I needed - we added those to
FAST_DEV
config flag but not toGATSBY_EXPERIMENTAL_FAST_DEV
env var route - I can drop those from here, but it would be weird forFAST_DEV
to only map toDEV_SSR
🤷