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

Improvements/gatsby 5 compatibility #21

Merged

Conversation

mcueto
Copy link
Contributor

@mcueto mcueto commented Dec 17, 2022

Hi guys!

I was updating to gatsby 5 from gatsby 4 and a graphql lib dependency error ocurred(multiple versions were being installed).

What was the problem it was generating?

The dependencies were conflicting and therefore the graphql schema for directus wasn't being generated.

imagen

imagen

How did I fix it?

I just updated the dependencies in the plugin to it's 5.x versions, that fixed the problem.

Which problems still exists with this approach?

There may be breaking changes if you try to install this package on a gatsby 4 website, I tested setting the requirements as ^4 and 4 - 5 and in both cases npm was installing the last version, so no gatsby 4 compatibility and that could be an issue.

I ended up setting them as 5.2.0 which is the last stable version of both dependencies, with that it works as expected on gatsby 5.

to install it use

npm install https://github.com/mcueto/gatsby-source-directus/tarball/improvements/gatsby-5-compatibility

and then you will be able to query your schema again

imagen

Note to the maintainers

I didn't update the package version because I didn't know whats the correct version to make it not backwards compatible or how you would solve those "breaking changes" aforementioned.

Cheers!

@rijkvanzanten
Copy link
Member

Should we try making the gatsby dependencies a peerDependency instead of a regular dependency so we can have the peer set to 4 / 5? 🤔 that way it's up to the project to make sure the correct versions exist for their projects and we can keep our package backwards compatible

@mcueto
Copy link
Contributor Author

mcueto commented Dec 18, 2022

Ok I will try that and then tell you what is the outcome

@mcueto
Copy link
Contributor Author

mcueto commented Dec 18, 2022

Added as peerDependency and tested on gatsby 4 and 5.

I also added some instructions to the readme.md file in order to let the users know what to do when installing for both versions.

(ps: I Think the readme file must be renamed to README.md.

Running on gatsby 4

imagen

Running on gatsby 5

imagen

@mcueto
Copy link
Contributor Author

mcueto commented Dec 21, 2022

Hi @freekrai please approve the workflow when possible, I have been testing gatsby 5 + the Slices API + directus and it works! 🥳

@freekrai
Copy link
Contributor

I'll be looking over this PR this week, also want to make sure the plug-in continues to work for 4 users as well so will be testing and posting comments

@msalmeron
Copy link

msalmeron commented Jan 17, 2023

Hi! @freekrai there's any issue or concern regarding this PR not being approved yet?
We've ongoing projects that will benefit from this, now as a workaround, we are installing the plugin directly from @mcueto fork
Thanks!

@rijkvanzanten rijkvanzanten merged commit d76e514 into directus:main Jan 17, 2023
@msalmeron
Copy link

Sorry to bother you here again here, but can be the npm package updated to include these changes?

"peerDependencies": {
"gatsby-source-filesystem": "4||5",
"gatsby-source-graphql": "4||5",
"eslint": "7||8"
Copy link
Member

@paescuj paescuj Jan 20, 2023

Choose a reason for hiding this comment

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

Sorry for this post-review, but why eslint as a peer dependency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will check it out during the weekend and then will add another comment in here.

@rijkvanzanten
Copy link
Member

Sorry to bother you here again here, but can be the npm package updated to include these changes?

Yes! Going up now 👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants