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

refactor(develop): expose APIs to suspend/resume webpack watching to gatsby internals #25236

Merged
merged 1 commit into from
Jun 26, 2020

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Jun 23, 2020

Description

This pull request just exposes ability to pause/resume webpack watching, but doesn't make any use of it yet. Main reason to split this to its own PR is the hairy TS situation:

Related Issues

Tracking PR for data-driven code-splitting - #24903

[CH9864]

@pieh pieh requested a review from a team as a code owner June 23, 2020 17:23
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jun 23, 2020
@pieh pieh changed the title refactor(develop): expose APIs to suspend/resume webpack watching refactor(develop): expose APIs to suspend/resume webpack watching to gatsby internals Jun 23, 2020
blainekasten
blainekasten previously approved these changes Jun 23, 2020
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.

Code looks good assuming it works as you want. I had a question about needing the returned key to be a function to access the webpack watching value. But if it works as you want, then I'm good with this!

@pieh pieh force-pushed the query-modules/expose-webpack-pause-resume branch from adcadab to b1ac123 Compare June 23, 2020 20:51
ascorbic
ascorbic previously approved these changes Jun 23, 2020
Copy link
Contributor

@ascorbic ascorbic left a comment

Choose a reason for hiding this comment

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

This is great. I'm really glad that you've unearthed this. It's going to be very useful.

@wardpeet wardpeet self-assigned this Jun 24, 2020
@wardpeet wardpeet added status: needs core review Currently awaiting review from Core team member topic: webpack/babel Webpack or babel and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Jun 24, 2020
@sidharthachatterjee sidharthachatterjee removed the status: needs core review Currently awaiting review from Core team member label Jun 24, 2020
@sidharthachatterjee sidharthachatterjee self-requested a review June 24, 2020 11:02
Copy link
Contributor

@sidharthachatterjee sidharthachatterjee left a comment

Choose a reason for hiding this comment

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

Nice work!

@wardpeet
Copy link
Contributor

@pieh code looks great! I don't see much value in abstracting this away from the larger PR as this doesn't show me usage of the functionality so I won't be able to see test failing, ... cause it's unused.

I know I can go to the larger PR but what's the point of abstracting it away?

@pieh
Copy link
Contributor Author

pieh commented Jun 25, 2020

@wardpeet the reason for exposing those methods is to be able to control when webpack recompiles (and emit hot update).

Current (master) method of running requires-writer in long running develop process is to listen to various actions and then call debounced write call. While not ideal, it's also not that problematic right now, because it just reacts to CREATE_PAGE (and similar) actions because it mostly just needs list of template components and templates remain mostly "fixed" and that's not changing over time that much (but especially in larger sites where createPages can take a while - this cause to rewrite (a)sync-requires multiple times during same createPages run.

Query modules to work need to add things to (a)sync-requires - this happen during query running. Because query running can also take a lot of time, this also mean rerunning it multiple times but this is worse than createPages because modules are "garbage collected" if not used, and because I do clean modules when query is marked as dirty, meaning that those extra runs might output broken code with missing modules that will be added eventually, but becasue of lot of asynchronity in all of this this can take a while and leave browser session broken with runtime errors overlays).

The follow up PR for this will look something like 2cc9227...df0c7fd (I'm still having some issues with e2e tests, so there likely be still some changes, but the idea of those changes is:
"don't run something if something we rely on is still running or we know that will run soon" and relationship between systems I defined as follow there:

  • schema rebuild (doesn't depend on anything from this list, so it will seemingly run anytime still)
  • createPages run (depend on schema rebuild)
  • query running (depend on both schema rebuild and createPages)
  • requires writer (depend on createPages) <- this is in master - with query modules this will depend on both createPages AND query running
  • webpack (re)compilation (depend on requires writer)
  • page data flush (depend on query running and webpack)

For this model to work we need to be able to control when things are running and webpack is only thing from the list that we don't control

Also just note that this follow up PR will be eventually made redundant with more robust solution in form of state machine (ideally I would wait for it, but it's still quite some time before it lands fully)

@pieh pieh dismissed stale reviews from sidharthachatterjee and ascorbic via ad21352 June 26, 2020 13:30
@pieh pieh force-pushed the query-modules/expose-webpack-pause-resume branch from b1ac123 to ad21352 Compare June 26, 2020 13:30
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.

Looking great!

@wardpeet wardpeet merged commit b73cc5d into master Jun 26, 2020
@wardpeet wardpeet deleted the query-modules/expose-webpack-pause-resume branch June 26, 2020 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: webpack/babel Webpack or babel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants