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: prevent reach-router dep requirement for v2 #557

Merged
merged 7 commits into from
Feb 23, 2023

Conversation

taty2010
Copy link
Contributor

@taty2010 taty2010 commented Feb 7, 2023

Summary

When using the current plugin version with Gatsby V2, users were running into this error:
Screenshot 2023-02-07 at 2 24 07 PM

With the previous Dynamic import update, there were issues that could have emerged when placing the deleteFunctions within the conditional. This new change converts the getApiHandler function (where reach-router is being called) into a string and prevents reach-router from being required as a dependency for v2 sites.

makeApiHandler contains getApiHandler and is only called when creating a function.

Test plan

  1. Can test it with a V2 site and make sure the above error doesn't appear

Relevant links (GitHub issues, Notion docs, etc.) or a picture of cute animal

Ref: #556

download

Standard checks:

  • Check the Deploy Preview's Demo site for your PR's functionality
  • Add docs when necessary

🧪 Once merged, make sure to update the version if needed and that it was
published correctly.

@taty2010 taty2010 self-assigned this Feb 7, 2023
@taty2010 taty2010 requested a review from a team February 7, 2023 20:40
@netlify
Copy link

netlify bot commented Feb 7, 2023

Deploy Preview for netlify-plugin-gatsby-demo canceled.

Name Link
🔨 Latest commit 328956e
🔍 Latest deploy log https://app.netlify.com/sites/netlify-plugin-gatsby-demo/deploys/63f7b63b9e0e8f00080dbf66

Copy link
Contributor

@nickytonline nickytonline left a comment

Choose a reason for hiding this comment

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

This looks good to me since we tested it together. 😎 I'll let someone else give some 👀 for approval as well though.

plugin/src/index.ts Outdated Show resolved Hide resolved
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.

Neat solution!

@ascorbic
Copy link
Contributor

ascorbic commented Feb 8, 2023

Could you give this PR a title that obeys the conventional commits format

plugin/src/index.ts Outdated Show resolved Hide resolved
@taty2010 taty2010 changed the title Tn/skip write function gatsby v2 fix: skip write function gatsby v2 Feb 8, 2023
@taty2010 taty2010 changed the title fix: skip write function gatsby v2 fix: skip write function for gatsby v2 Feb 8, 2023
@taty2010 taty2010 changed the title fix: skip write function for gatsby v2 fix: dynamically import write + delete function for gatsby v2 Feb 13, 2023
@taty2010 taty2010 requested a review from ascorbic February 17, 2023 16:13
@taty2010 taty2010 changed the title fix: dynamically import write + delete function for gatsby v2 fix: prevent reach-router dep requirement for v2 Feb 17, 2023
@taty2010 taty2010 removed the request for review from ascorbic February 21, 2023 19:37
@kodiakhq kodiakhq bot merged commit 6d541d1 into main Feb 23, 2023
@kodiakhq kodiakhq bot deleted the tn/skip-writeFunction-gatsbyV2 branch February 23, 2023 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants