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.

Sign up for GitHub

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

Jump to bottom

feat(develop): add query on demand behind feature flag #28127

Merged
merged 9 commits into from
Nov 17, 2020
Merged

feat(develop): add query on demand behind feature flag #28127

merged 9 commits into from
Nov 17, 2020

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Nov 17, 2020

This is final piece for implementation of query on demand feature flag

It relies on #27939 ( so target branch is that until it gets merged )

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Nov 17, 2020
@pieh pieh force-pushed the qod/connect-all-the-dots branch from 10ca94e to 2d55579 Compare November 17, 2020 14:24
@pieh pieh removed the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Nov 17, 2020
@delete-merged-branch delete-merged-branch bot deleted the branch master November 17, 2020 15:36
@vladar
Copy link
Contributor

vladar commented Nov 17, 2020

I like the branch name 😉

@pieh pieh force-pushed the qod/connect-all-the-dots branch from 2d55579 to 50cd5c5 Compare November 17, 2020 15:57
@pieh pieh marked this pull request as ready for review November 17, 2020 16:02
@pieh pieh changed the base branch from vladar/page-data-util to master November 17, 2020 16:02
@pieh pieh force-pushed the qod/connect-all-the-dots branch from 50cd5c5 to 91313e6 Compare November 17, 2020 16:25
vladar
vladar suggested changes Nov 17, 2020
View reviewed changes
Copy link
Contributor

@vladar vladar left a comment

Choose a reason for hiding this comment

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

Looks great. One thing that requires attention is a leftover for trackDirtyQuery in the query reducer as it is unconditional.

packages/gatsby/cache-dir/dev-loader.js Show resolved Hide resolved
packages/gatsby/cache-dir/dev-loader.js Show resolved Hide resolved
packages/gatsby/cache-dir/dev-loader.js Show resolved Hide resolved
packages/gatsby/src/redux/types.ts Show resolved Hide resolved
packages/gatsby/src/services/calculate-dirty-queries.ts Outdated Show resolved Hide resolved
packages/gatsby/src/services/calculate-dirty-queries.ts Outdated Show resolved Hide resolved
packages/gatsby/src/redux/reducers/queries.ts Outdated Show resolved Hide resolved
packages/gatsby/src/redux/reducers/queries.ts Show resolved Hide resolved
packages/gatsby/src/utils/websocket-manager.ts Outdated Show resolved Hide resolved
pieh and others added 9 commits November 17, 2020 18:17
@pieh
skip writing page-data if query results are not fresh
d9fc06f
@pieh
emit websocket message with list of dirty queries
b0c2239
@pieh
handle dirty queries websocket message in runtime
2f12978
@pieh
actually enable query on demand
84d8ba4
@pieh
force clear GATSBY_EXPERIMENTAL_QUERY_ON_DEMAND flag for commands oth…
add85dd 
…er than develop
@pieh @vladar
Apply suggestions from code review
353b24a 
Co-authored-by: Vladimir Razuvaev <[email protected]>
@pieh @vladar
Apply suggestions from code review
3c9de96 
Co-authored-by: Vladimir Razuvaev <[email protected]>
@pieh
TIL: process.env.foo = undefined result in 'undefined' string being s…
fd18243 
…et and not resetting it
@pieh
websocket: don't discard dirty queries ids before websocket is initia…
adb295f 
…lized

user can have satured runtime caches and restart dev server which means they might
miss some invalidation messages and would need to manually refresh browser
@pieh pieh force-pushed the qod/connect-all-the-dots branch from cf4f6dc to adb295f Compare November 17, 2020 17:22
vladar
vladar approved these changes Nov 17, 2020
View reviewed changes
Copy link
Contributor

@vladar vladar left a comment

Choose a reason for hiding this comment

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

Huge work! Let's ship it 🚢

@vladar vladar added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Nov 17, 2020
@gatsbybot gatsbybot merged commit 088eef4 into master Nov 17, 2020
@delete-merged-branch delete-merged-branch bot deleted the qod/connect-all-the-dots branch November 17, 2020 19:09
@kaocher82 kaocher82 mentioned this pull request Jul 22, 2022
Open
This was referenced Jul 22, 2022
Open
Open
@nidhi42 nidhi42 mentioned this pull request Jul 22, 2022
Open
This was referenced Jul 22, 2022
Open
Open
Open
@kaocher82 kaocher82 mentioned this pull request Jul 23, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vladar vladar vladar approved these changes

Assignees
No one assigned
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pieh @vladar @gatsbybot