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

fix(gatsby): validate local plugin options schema #29787

Merged

Conversation

kamranayub
Copy link
Contributor

@kamranayub kamranayub commented Feb 26, 2021

Description

Fix for pluginOptionsSchema not being validated for locally resolved plugins.

Changes

  • Use resolvePlugins when validating options
  • Add integration test for local plugin loading/validation
  • Add integration test to CI configuration

Tasks

  • Provide failing test case for local plugin (with require.resolve)
  • Provide failing test case for local plugin (without require.resolve)
  • Backport for v2 🆘 🤚 Need assistance/guidance with this.

Documentation

Related Issues

Fixes #29785
Related to #27242

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Feb 26, 2021
@kamranayub
Copy link
Contributor Author

Will need to handle when using require.resolve as the IPluginRefObject looks like this:

 {
      resolve: 'E:\\Development\\Contrib\\gatsby\\packages\\gatsby\\src\\bootstrap\\load-plugins\\__tests__\\fixtures\\local-plugin\\index.js',
      options: { optionalString: 1234 }
    }

So the following line in validate.ts won't work like you'd expect:

gatsbyNode = require(`${plugin.resolve}/gatsby-node`)

@kamranayub
Copy link
Contributor Author

It looks like there's a snapshot failing unrelated to this PR; it was failing in master too when I ran this suite.

I took a cue from resolvePlugin in load to check whether the path is absolute and if so, to grab the dirname and use slash to normalize paths for Windows/Linux.

@kamranayub
Copy link
Contributor Author

Integration test passes locally (when using linked package)

image

@kamranayub
Copy link
Contributor Author

I am not sure if an e2e test is more appropriate for this or my integration test, let me know 😄

@kamranayub
Copy link
Contributor Author

kamranayub commented Feb 26, 2021

I realized local-plugin was resolving to the require.resolve and the other one was being dropped in the integration test; split them up, now I can see there's still an issue with local paths.

image

Will continue investigating proposed fix tomorrow. Fixing require.resolve will unblock my course dev/recording since I can use local patch (🙈) and uptake this when it's backported to V2 officially.

@kamranayub kamranayub marked this pull request as draft February 26, 2021 05:31
@LekoArts LekoArts added topic: plugins Related to plugin system, themes & catch-all for plugins that don't have a label and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Feb 26, 2021
@LekoArts
Copy link
Contributor

Hi, thanks for the PR!

Backport for v2 🆘 🤚 Need assistance/guidance with this.

We'll handle this :)

I am not sure if an e2e test is more appropriate for this or my integration test, let me know

Integration test is fine

I did not see any docs in the integration test README but to actually test against my WIP changes, I had to use yarn link. If I didn't, I was getting an error with missing node modules

For local development we use yarn workspaces and our own CLI gatsby-dev-cli: #29787

So to copy your changes to the integration test, you can run gatsby-dev inside the directory, while running yarn watch --scope=gatsby in the monorepo itself.

@kamranayub
Copy link
Contributor Author

@LekoArts Thanks, that worked nicely 👍

@kamranayub kamranayub force-pushed the gatsby/fix-local-plugin-schema-29785-v3 branch from 93f1de9 to 5bce9d4 Compare March 1, 2021 01:59
@kamranayub kamranayub marked this pull request as ready for review March 1, 2021 02:04
@kamranayub
Copy link
Contributor Author

I think the tests that are failing are unrelated; I see the same ones failing in other approved PRs too. 😞 But this should be ready to go.

@kamranayub kamranayub force-pushed the gatsby/fix-local-plugin-schema-29785-v3 branch 2 times, most recently from 44d5806 to b59e5fc Compare March 5, 2021 02:56
@kamranayub
Copy link
Contributor Author

Just confirming that this works when using gatsby-dev in my demo app:

image

@kamranayub kamranayub force-pushed the gatsby/fix-local-plugin-schema-29785-v3 branch from b59e5fc to f0a63a9 Compare March 8, 2021 21:36
@kamranayub
Copy link
Contributor Author

@LekoArts Anything else I need to do? Thanks!

@sidyes
Copy link

sidyes commented Mar 14, 2021

Is there anyway this can be pushed as the solution has already been provided as a PR?

@kamranayub
Copy link
Contributor Author

I'll be recording my demos next week and this would be amazing to get in so I don't have to keep using local dev fork 😄

@LekoArts LekoArts self-assigned this Mar 23, 2021
@LekoArts
Copy link
Contributor

Hey @kamranayub - sorry for the delay here, we were (and are) busy with follow-up work of the v3 launch.
We'll release the next minor on March 30, 2021 and I'll see to get this reviewed and merged before that :)

@kamranayub
Copy link
Contributor Author

Awesome, thanks!

Copy link
Contributor

@LekoArts LekoArts left a comment

Choose a reason for hiding this comment

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

Thanks!

@LekoArts LekoArts merged commit 096eb38 into gatsbyjs:master Mar 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: plugins Related to plugin system, themes & catch-all for plugins that don't have a label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[gatsby] validate plugin options does not run against a locally resolved plugin
4 participants