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

cache instance for subplugins #8788

Closed
pieh opened this issue Oct 4, 2018 · 9 comments · Fixed by #10146
Closed

cache instance for subplugins #8788

pieh opened this issue Oct 4, 2018 · 9 comments · Fixed by #10146
Labels
type: bug An issue or pull request relating to a bug in Gatsby

Comments

@pieh
Copy link
Contributor

pieh commented Oct 4, 2018

After implementing separate cache for each plugin, there is issue with cache instance for subplugins:

  • subplugins can implement their own API hooks and there they get cache instance for that subplugin
  • way parent plugins interact with subplugins is most often implementing most of subplugin functionality in something like index.js in this is called by parent plugin and often we pass cache from parent plugin to subplugin - for example in gatsby-transformer-remark:
    Promise.each(pluginOptions.plugins, plugin => {
    const requiredPlugin = require(plugin.resolve)
    if (_.isFunction(requiredPlugin)) {
    return requiredPlugin(
    {
    markdownAST,
    markdownNode,
    getNode,
    files: fileNodes,
    pathPrefix,
    reporter,
    cache,
    },
    plugin.pluginOptions
    )
    } else {
    return Promise.resolve()
    }

Because of that different cache instances are used in subplugins gatsby-node.js and index.js files.

You can check https://github.com/pieh/gatsby-subplugins-cache reproduction (most important thing is in https://github.com/pieh/gatsby-subplugins-cache/tree/master/plugins/gatsby-remark-test ).

I think we would need to pass cache manager to plugins as well so parent plugins can pass correct cache instances to subplugins

/cc @DSchau @raae

@pieh pieh added type: bug An issue or pull request relating to a bug in Gatsby no triage labels Oct 4, 2018
@KyleAMathews
Copy link
Contributor

Or do we really need different instances per plugin? My thought was that we'd track cache IDs by plugin so could purge cache items that way. Having plugins manage cache instances sounds like a confusing and error prone API.

@DSchau
Copy link
Contributor

DSchau commented Oct 4, 2018

The plugin name is effectively the id, and that's what sets everything up!

Having plugins manage cache instances

I don't think we want that either! The cache process should be transparent to plugins, the API hasn't/won't change!

@pieh
Copy link
Contributor Author

pieh commented Oct 4, 2018

Or do we really need different instances per plugin? My thought was that we'd track cache IDs by plugin so could purge cache items that way.

Tracking cache keys by plugin will have similar issues (at least for this issue) as we are passing cache object (which would be what ultimately would have to track what cache keys belong to what plugin). With separate cache instances we get this tracking for free as every plugin just have its own cache, so we can just purge entire plugin cache and not filter out cache keys belonging to plugin we want to purge.

Having plugins manage cache instances sounds like a confusing and error prone API.

I do agree on that. This is only needed for some subplugins (when we pass cache instance to them from parent plugin and when they implement their own gatsby node API hooks).

Maybe alternative solution would be to handle subplugins caches in gatsby core and pass them as part of subplugins array - but it would ultimately be up to parent plugins to make use of that. Unless we abstract subplugins even more so parent plugins don't call them directly, but that would be breaking change at this point.

@pieh
Copy link
Contributor Author

pieh commented Oct 4, 2018

Also just to be clear - right now this change should only be used by gatsby-transformer-remark plugin when calling subplugins - other plugins should use cache objects they get from gatsby core

@chmac
Copy link
Contributor

chmac commented Nov 5, 2018

@pieh @KyleAMathews @DSchau Any update on this one? It seems like the implementation of the gatsby-remark-oembed plugin (which looks awesome) is blocked by this. Anything I can do to help out? It seems like we're blocked waiting for a decision on the best approach.

@amykapernick
Copy link

Any update on this? This bug also means I'm stuck on 2.0.17 because I'm using gatsby-remark-oembed but now I'm getting a different error error Plugin gatsby-transformer-sharp returned an error TypeError: getNodesByType is not a function which according to #9533 needs to use 2.0.33 or higher to resolve

@DSchau
Copy link
Contributor

DSchau commented Nov 21, 2018

@amykapernick a few of us are out with the holiday until early next week, so I can prioritize this as something to take a look at first thing next week. Hopefully that's OK! IF not PRs are more than welcome in the interim!

@amykapernick
Copy link

@DSchau I would love to help out, but unfortunately I'm not quite at a point where I can contribute to this kind of bug. When I do get there though, I'm keen to give back to Gatsby because it's amazing and I love the work you're all doing!

@raae
Copy link

raae commented Dec 20, 2018

What is the status of this issue? I see there is a pull request, but do not understand if it's being approved or not.

If this will take a long time I will have to start looking into a workaround for my plugin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug An issue or pull request relating to a bug in Gatsby
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants