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: exports GraphQLArmorConfig type globally #313

Conversation

dthyresson
Copy link
Contributor

This PR fixes/improves the way one can use the GraphQLArmorConfig type by exporting it globally.

Prior to this PR, if you wanted to use the type, one would have to import it this way:

import type { GraphQLArmorConfig } from '@escape.tech/graphql-armor/dist/declarations/src/config'

Notice that the import uses the dist.

Now, one should be able to

import type { GraphQLArmorConfig } from '@escape.tech/graphql-armor'

Since

import type { GraphQLArmorConfig } from '../src/index';

is exported globally.

A quick sanity test was added to ensure the export exists at the global/index level.

@changeset-bot
Copy link

changeset-bot bot commented Jan 6, 2023

🦋 Changeset detected

Latest commit: 605ff34

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
@escape.tech/graphql-armor Minor
@escape.tech/graphql-armor-types Minor
@escape.tech/graphql-armor-character-limit Patch
@escape.tech/graphql-armor-cost-limit Patch
@escape.tech/graphql-armor-max-aliases Patch
@escape.tech/graphql-armor-max-depth Patch
@escape.tech/graphql-armor-max-directives Patch
@escape.tech/graphql-armor-max-tokens Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codecov-commenter
Copy link

codecov-commenter commented Jan 6, 2023

Codecov Report

Base: 91.90% // Head: 91.90% // No change to project coverage 👍

Coverage data is based on head (e65ec4e) compared to base (de13f85).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #313   +/-   ##
=======================================
  Coverage   91.90%   91.90%           
=======================================
  Files          17       17           
  Lines         284      284           
  Branches       69       69           
=======================================
  Hits          261      261           
  Misses         23       23           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@nullswan
Copy link
Member

nullswan commented Jan 6, 2023

looks nice, I think we could even move configuration into the types package.

@dthyresson
Copy link
Contributor Author

dthyresson commented Jan 6, 2023

looks nice, I think we could even move configuration into the types package.

Oh, so no config in packages/graphql-armor/src but move all that into types and update accordingly.

Imagine this would be a major bump rather than minor.

And then to use

import type { GraphQLArmorConfig } from '@escape.tech/graphql-armor-types'

?

@dthyresson
Copy link
Contributor Author

I tried to make that change ti move all into types.

I haven't looked to see if this requires any documentation changes yet.

If this move is what you are looking for, then I'll check for docs.

@dthyresson
Copy link
Contributor Author

Also, should these docs https://escape.tech/graphql-armor/docs/api/types be updated to reflect the additional exported types? I think so and will update.

@dthyresson
Copy link
Contributor Author

Also, should these docs https://escape.tech/graphql-armor/docs/api/types be updated to reflect the additional exported types? I think so and will update.

Added docs for config types.

@nullswan
Copy link
Member

nullswan commented Jan 9, 2023

looks nice, I think we could even move configuration into the types package.

Oh, so no config in packages/graphql-armor/src but move all that into types and update accordingly.

Imagine this would be a major bump rather than minor.

And then to use

import type { GraphQLArmorConfig } from '@escape.tech/graphql-armor-types'

?

Minor bump should fit, this type shouldn't be used as it is not exported.

@nullswan nullswan force-pushed the dt-export-graphql-armor-config-type branch from 6bd4199 to 605ff34 Compare January 9, 2023 08:26
@nullswan
Copy link
Member

nullswan commented Jan 9, 2023

Hey @dthyresson !

I updated the internal type usage to depend of the types package.
& rebased

Thanks a lot for updating the documentation accordingly.

Any additional changes in mind?

@dthyresson
Copy link
Contributor Author

dthyresson commented Jan 9, 2023

Thanks a lot for updating the documentation accordingly.

Glad to!

Any additional changes in mind?

Not for this PR.

Merging and then will use latest package in RedwoodJS in redwoodjs/redwood#7291

Edit: I forgot. I can't merge :)

@nullswan nullswan merged commit db50253 into Escape-Technologies:main Jan 9, 2023
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

3 participants