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.

Sign up for GitHub

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

Jump to bottom

add netlify handler #96

Merged
merged 1 commit into from
Jul 17, 2023
Merged

add netlify handler #96

merged 1 commit into from
Jul 17, 2023

Conversation

acao
Copy link
Member

@acao acao commented Jul 5, 2023 •
edited

  • use implementation (for review)
  • docs

@acao
Copy link
Member Author

acao commented Jul 5, 2023 •
edited

Issue #71 seems to be impacting this, otherwise everything looks good to go!

When yarn linking with graphql/graphiql and running netlify-cli this works!

@acao acao force-pushed the netlify-handler branch 2 times, most recently from 38cb9e6 to 69d0beb Compare July 5, 2023 13:50
@acao acao mentioned this pull request Jul 5, 2023
Closed
4 tasks
enisdenjo
enisdenjo requested changes Jul 5, 2023
View reviewed changes
Copy link
Member

@enisdenjo enisdenjo 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! Left a few remarks, also please also run yarn gendocs and commit the changes.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
enisdenjo
enisdenjo reviewed Jul 5, 2023
View reviewed changes
Copy link
Member

@enisdenjo enisdenjo left a comment

Choose a reason for hiding this comment

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

Also, please add netlify to the exports map in the package.json.

enisdenjo
enisdenjo requested changes Jul 5, 2023
View reviewed changes
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@acao
Copy link
Member Author

acao commented Jul 6, 2023

@enisdenjo I'm seeing a weird issue where the compiled .mjs files have esModule: true, but still have require() ala commonjs, is there a tooling bug? this seems to break the netlify-cli serve command's esbuild run

image

@acao acao force-pushed the netlify-handler branch 2 times, most recently from b98b4ff to db14972 Compare July 6, 2023 12:12
@enisdenjo
Copy link
Member

Have you tried cleaning/removing the lib directory and then building again?

@acao
Copy link
Member Author

acao commented Jul 6, 2023

@enisdenjo yup, that does the trick!

should I add to the example a step to detect http method and error on anything besides GET or POST or is that something that graphql-http would handle?

@enisdenjo
Copy link
Member

graphql-http handles the request method. The idea is to have the handler be "pure", it gets a request and returns a response.

enisdenjo
enisdenjo reviewed Jul 7, 2023
View reviewed changes
README.md Outdated Show resolved Hide resolved
enisdenjo
enisdenjo reviewed Jul 7, 2023
View reviewed changes
Copy link
Member

@enisdenjo enisdenjo left a comment

Choose a reason for hiding this comment

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

Please also rebase. Note that I've pushed the lighthouse readme changes on the main branch. So this PR should have a smaller changeset afterwards.

@acao acao force-pushed the netlify-handler branch 2 times, most recently from 2d1863c to 53897b1 Compare July 7, 2023 16:21
enisdenjo
enisdenjo reviewed Jul 7, 2023
View reviewed changes
README.md Outdated Show resolved Hide resolved
@acao acao force-pushed the netlify-handler branch 2 times, most recently from 8768212 to 23bab49 Compare July 11, 2023 22:14
@enisdenjo enisdenjo mentioned this pull request Jul 14, 2023
Merged
enisdenjo
enisdenjo requested changes Jul 14, 2023
View reviewed changes
Copy link
Member

@enisdenjo enisdenjo left a comment

Choose a reason for hiding this comment

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

Hey hey @acao, this looks great! Just one thing before merging, can you please rebase again (because I landed this). Sorry for the annoyance. 😬

@acao
Copy link
Member Author

acao commented Jul 14, 2023 •
edited

@enisdenjo haha, can relate with the PR traffic. rebased, thanks so much!

@enisdenjo
Copy link
Member

Not really sure what happened, but the lighthouse change is still in this PR while the same is already on main (ef8b311). Have you pulled main first?

@enisdenjo
Copy link
Member

You're missing 9 commits from the main on your fork.

@acao
Copy link
Member Author

acao commented Jul 14, 2023

@enisdenjo ahhh, I'm not used to working from forks. i rebased from origin (my fork) and not upstream

@acao
Copy link
Member Author

acao commented Jul 14, 2023

weird, after rebasing and locally squashing, and running yarn, i tried running yarn gendocs and it showed me a diff that was unrelated to this PR? perhaps you'll catch this upstream?

also it seems something with the github cache failed with something related to a lighthouse docker image

@acao
Copy link
Member Author

acao commented Jul 17, 2023 •
edited

@enisdenjo it is rebased off the latest upstream main branch, what should be done?

I already pulled it into our netlify function directly and it works great

enisdenjo
enisdenjo approved these changes Jul 17, 2023
View reviewed changes
Copy link
Member

@enisdenjo enisdenjo left a comment

Choose a reason for hiding this comment

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

This is it, great work @acao and thanks!

@enisdenjo enisdenjo merged commit 2b1ab14 into graphql:main Jul 17, 2023
25 of 28 checks passed
@acao
Copy link
Member Author

acao commented Jul 17, 2023

thanks @enisdenjo, it's been great working with you, and working in this repo is really nice!

enisdenjo pushed a commit that referenced this pull request Jul 17, 2023
@semantic-release-bot
chore(release): 🎉 1.21.0 [skip ci]
823bf11 
# [1.21.0](v1.20.0...v1.21.0) (2023-07-17)

### Bug Fixes

* **client:** `graphql` module is not required for runtime ([#102](#102)) ([9049f31](9049f31))

### Features

* Use Netlify Functions ([#96](#96)) ([2b1ab14](2b1ab14))
@enisdenjo
Copy link
Member

🎉 This PR is included in version 1.21.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@enisdenjo enisdenjo added the released Has been released and published label Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@enisdenjo enisdenjo enisdenjo approved these changes

Assignees
No one assigned
Labels
released Has been released and published
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@acao @enisdenjo