feat(gatsby-source-wordpress): support multiple instances of plugin #38119
Conversation
gatsbot
bot
added
the
status: triage needed
| "@rematch/core": "^1.4.0", | ||
| "@rematch/immer": "^1.2.0", | ||
| "@rematch/core": "^2.2.0", | ||
| "@rematch/immer": "^2.1.3", |
There was a problem hiding this comment.
The old version of rematch had completely broken TS types
There was a problem hiding this comment.
nice! it did indeed. I looked at upgrading around a year ago and went through the whole process only to discover rematch was no longer working and I didn't have more time to figure out why 🤦 hopefully you fared better here!
There was a problem hiding this comment.
good thing is, while they're odd, the tests are fairly comprehensive, so if there's any problem it'll definitely show up in the tests
There was a problem hiding this comment.
The good stuff is in here
| ) | ||
| } | ||
|
|
||
| const store = (): Store => asyncLocalStorage.getStore() |
There was a problem hiding this comment.
We change store from an exported store to a function which returns the correct store for the context
| effects: () => { | ||
| return {} | ||
| }, |
There was a problem hiding this comment.
There's a bug in the Rematch types that mean that if there are no effects, it infers the type of reducers incorrectly, so we need to add all these empty effects
| if (!STORE_MAP.has(typePrefix)) { | ||
| STORE_MAP.set( | ||
| typePrefix, | ||
| init({ | ||
| models, | ||
| plugins: [immerPlugin<IRootModel>()], | ||
| }) | ||
| ) | ||
| } | ||
|
|
||
| const store = STORE_MAP.get(typePrefix) |
There was a problem hiding this comment.
We get the instance of the store, keyed by the plugin's type prefix
There was a problem hiding this comment.
This is great, I love this implementation. async storage is perfect for this.
Looking at this block now I'm wondering if we should throw an error if there are multiple instances that have the same prefix. What do you think? Having 2 instances with the same prefix will break most things in whichever of those instances runs first
There was a problem hiding this comment.
otherwise I think we could possibly get at the plugin instance ID and use that to index each store. In that case multiple instances with the same prefix will work and all their nodes will be mixed together
There was a problem hiding this comment.
That's a good idea. I think using the instance ID as the key means we could also check that there if there's a prefix collision
| const store = STORE_MAP.get(typePrefix) | ||
|
|
||
| return asyncLocalStorage.run(store, async () => | ||
| hook(helpers, pluginOptions) |
There was a problem hiding this comment.
Run the hook using AsyncLocalStorage, passing the correct store instance
| const runApiSteps = (steps: Array<Step>, apiName: string): IGatsbyApiHook => | ||
| wrapApiHook( | ||
| async ( | ||
| helpers: GatsbyNodeApiHelpers, | ||
| pluginOptions: IPluginOptions | ||
| ): Promise<void> => runSteps(steps, helpers, pluginOptions, apiName) | ||
| ) |
There was a problem hiding this comment.
We wrap the function in wrapApiHook, which adds the handling for AsyncLocalStorage
LekoArts
added
topic: source-wordpress
ascorbic
force-pushed
the
wp-store-context
branch
from
def75f1
to
6e2b9b0
Compare
| import * as steps from "./steps" | ||
|
|
||
| const pluginInitApiName = findApiName(`onPluginInit`) |
There was a problem hiding this comment.
As this only checks for one method name it's a noop, so can be removed
| const previousTypeDefinitions = await cache.get(`previousTypeDefinitions`) | ||
| const previousTypeDefsKey = withPluginKey(`previousTypeDefinitions`) | ||
|
|
||
| const previousTypeDefinitions = await cache.get(previousTypeDefsKey) |
There was a problem hiding this comment.
Include the plugin type prefix in the cache key
ascorbic
changed the title
| expect(jobsManager.enqueueJob).toMatchSnapshot() | ||
| expect(jobsManager.enqueueJob).toHaveBeenCalledWith({ |
There was a problem hiding this comment.
This test was failing because it was snapshotting a Promise object, and using AsyncLocalStorage in other tests meant that there were added internal fields on global Promise. The object it's snapshotting is a jest mock, so it makes more sense to use the actual mock assertions instead of snapshotting its internal state
| })() | ||
| }) | ||
| beforeAll( | ||
| withGlobalStore(done => { |
There was a problem hiding this comment.
I think the tests and test utils shouldn't need withGlobalStore since they're int tests that are closer to e2e tests. gatsby develop is run in a subprocess and then tests make assertions against queries to the gql server so any store scoping will happen inside that subprocess.
I could be missing something of course, did this fix an issue?
There was a problem hiding this comment.
Oh is it cause the tests weirdly use fetchGraphQL from within source-wordpress?
There was a problem hiding this comment.
Yeah, exactly. I've added it in places where we're doing deep imports from the plugin
There was a problem hiding this comment.
Maybe it would be better to refactor out those imports 🤔
There was a problem hiding this comment.
definitely, I did that for content engine in another PR. I can follow up after this PR and bring my changes over from there
| field.type.name = `JSON` | ||
| return { | ||
| ...field, | ||
| type: new GraphQLScalarType({ | ||
| name: `JSON`, | ||
| }), | ||
| } |
There was a problem hiding this comment.
field.type, type.name etc are now immutable in graphql.js, so these were throwing errors in tests.
ascorbic
force-pushed
the
wp-store-context
branch
from
677f3c9
to
d880c0a
Compare
| /** | ||
| * | ||
| * @param {Object} options | ||
| * @param {string} options.url | ||
| * @param {string} options.query | ||
| * @param {Object} options.variables | ||
| * @param {Object} options.headers | ||
| * | ||
| * @returns {Promise<Object>} | ||
| */ | ||
| exports.fetchGraphql = async ({ url, query, variables = {}, headers = {} }) => { |
There was a problem hiding this comment.
Same signature as the one we were previously importing from the plugin
| export const buildTypeName = (name, prefix) => { | ||
| if (!name || typeof name !== `string`) { | ||
| return null | ||
| } | ||
|
|
||
| const { | ||
| schema: { typePrefix: prefix }, | ||
| } = getPluginOptions() | ||
| if (!prefix) { | ||
| prefix = getPluginOptions().schema.typePrefix | ||
| } |
There was a problem hiding this comment.
Allowing the prefix to be passed in rather than needing to call getPluginOptions means we can remove the need to use the store at query time.
| typeDef.resolveType = node => | ||
| node?.__typename ? buildTypeName(node.__typename) : null | ||
| node?.__typename ? buildTypeName(node.__typename, prefix) : null | ||
| } |
There was a problem hiding this comment.
During develop, this resolver is called outside of the async scope of the createSchemaCustomization api, so we'll instead pass the prefix in from this scope
|
@TylerBarnes do you know what would cause this failure? |
|
@ascorbic for the yoast test Though it also looks like a bunch of the Gatsby schema differences in the second failed test are for Yoast types 🤔 so maybe there's a change in here somewhere that's unexpectedly changing how the schema is generated? |
|
@TylerBarnes Thanks. The Yoast one was fixed by 58ba99e but I've had less luck with the other test. I'm not sure that one is Yoast-related, because some of the types are unrelated. Can you explain a bit more about the test? What would have changed the schema? |
|
@ascorbic unfortunately as I'm sure you've experienced the codebase for this plugin is complicated to say the least. |
|
@TylerBarnes I think I've solved it! 29dd03e |
ascorbic
force-pushed
the
wp-store-context
branch
from
fccc74c
to
29dd03e
Compare
|
Oh, and thanks for the comprehensive tests that caught this, @TylerBarnes ! |
|
@ascorbic @TylerBarnes I'm having some async issues after the upgrade to 7.11.0 of gatsby-source-wordpress. I resolved this after downgrading to 7.10.1, and based on the error, this update is the cause.
This causes the worker to exit without finishing. Would you happen to have any insights on what's going on here, purhapes a way to debug to understand why it's losing the context of the store? |
@ThyNameIsMud Were you able to fix this? I've been running into this issue for months as I'm trying to upgrade from v3 to v5. |
Description
Currently the WordPress plugin can't support multiple instances because it uses a global redux store. This PR instead scopes the store according to the type prefix. It uses AsyncLocalStorage to ensure that each api call uses the correct store. It additionally scopes the cache, by including the prefix in the cache key.
Documentation
Tests
Related Issues