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

feat: enable bypassing persisted operations #4

Merged
merged 18 commits into from
Mar 16, 2021
Merged

feat: enable bypassing persisted operations #4

merged 18 commits into from
Mar 16, 2021

Conversation

leo91000
Copy link
Contributor

@leo91000 leo91000 commented Feb 14, 2021

Description

This PR is a feature originally discussed in #2

It allows for an additional postgraphile option allowUnpersistedQueries (req: any): boolean for bypassing persisted queries (you usually want that in development environment).

Performance impact

This feature adds code that is executed if the query hash is not found. It does the following :

  • Get allowUnpersistedQueries
  • If allowUnpersistedQueries returns true, it get the query from payload and returns it

Security impact

Maybe we should add a warning for developpers to inform that an unpersisted query has been sent ?

Checklist

  • My code matches the project's code style and yarn lint:fix passes.
  • I've added tests for the new feature, and yarn test passes.
  • I have detailed the new feature in the relevant documentation.
  • I have added this feature to 'Pending' in the RELEASE_NOTES.md file (if one exists).
  • If this is a breaking change I've explained why.

Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

Excellent start 🙌

README.md Outdated Show resolved Hide resolved
RELEASE_NOTES.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@leo91000 leo91000 requested a review from benjie February 16, 2021 09:36
benjie
benjie previously requested changes Feb 16, 2021
Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

Looking good; almost there 👍

RELEASE_NOTES.md Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
@leo91000 leo91000 requested a review from benjie February 16, 2021 11:31
Copy link
Member

@benjie benjie 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 great. I'll need to test it before I can merge it. What do you think about enabling it to receive a boolean as well as a function?

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
@leo91000
Copy link
Contributor Author

@benjie I added the option to pass directly a boolean

Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

Great; I think I might want to revise the type of payload from any to something a bit tighter; but other than that this looks solid 👍

Leave it with me now.

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
@leo91000
Copy link
Contributor Author

Thanks, you are awesome ! 💯

@benjie
Copy link
Member

benjie commented Mar 4, 2021

(I haven't forgotten about this.)

@leo91000
Copy link
Contributor Author

leo91000 commented Mar 5, 2021

(I haven't forgotten about this.)

No problem take your time

@benjie
Copy link
Member

benjie commented Mar 16, 2021

Hey @leo91000 can you grant me permission to push to this branch?

@leo91000
Copy link
Contributor Author

Hey @leo91000 can you grant me permission to push to this branch?

I am not sure how to achieve that 🤔. I invited you to the repository, is this the way ?

@benjie
Copy link
Member

benjie commented Mar 16, 2021

That works; in future you can just check a checkbox in the right sidebar "Allow edits from maintainers":

https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork

@benjie benjie changed the title feat: Enable bypassing persisted operations feat: enable bypassing persisted operations Mar 16, 2021
@benjie benjie merged commit 59e927e into graphile:main Mar 16, 2021
@benjie
Copy link
Member

benjie commented Mar 16, 2021

Release in v0.1.0 🎉

@leo91000
Copy link
Contributor Author

Yay 🎉 ! Thank you for taking time for this !

@benjie
Copy link
Member

benjie commented Mar 18, 2021

Thanks for writing it; it was the remaining thing stopping me from moving it to 0.x from 0.0.x 😁

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.

None yet

2 participants