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

migrate graphiql-explorer into the repo, ts conversion #3374

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

acao
Copy link
Member

@acao acao commented Jul 26, 2023

  • manually converted from flow, down to ~4 errors with strict: false, and with strict: true it is about ~51
  • disable, eslint and spellcheck for these files temporarily to test the integration
  • adopting from the original repo https://github.com/OneGraph/graphiql-explorer as per previous discussions with onegraph
  • resolve graphiql-explorer locally for the plugin build
  • split into multiple files for maintainability

to preview, see:

https://deploy-preview-3374--graphiql-test.netlify.app/webpack

also provides a typedoc

CC

@dwwoelfel and/or @sgrove , how would you like to go about this? Attribution in the readme and whatever license you chose? Or should we publish it as @graphiql/explorer under a seperate NPM module?

BCC @SachaG

@changeset-bot
Copy link

changeset-bot bot commented Jul 26, 2023

⚠️ No Changeset found

Latest commit: 2340812

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@github-actions
Copy link
Contributor

The latest changes of this PR are not available as canary, since there are no linked changesets for this PR.

@netlify
Copy link

netlify bot commented Jul 26, 2023

Deploy Preview for graphiql-test ready!

Name Link
🔨 Latest commit f8cfaf0
🔍 Latest deploy log https://app.netlify.com/sites/graphiql-test/deploys/64c6944a247a680008f47921
😎 Deploy Preview https://deploy-preview-3374--graphiql-test.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@acao acao force-pushed the rehome-graphiql-explorer branch 7 times, most recently from f44d5f7 to 0f75f9f Compare July 26, 2023 20:23
@acao acao changed the title begin migrating graphiql-explorer into the repo, ts conversion migrate graphiql-explorer into the repo, ts conversion Jul 26, 2023
@acao acao added the graphiql label Jul 27, 2023
Explorer as ExplorerInner,
ExplorerWrapper as default,
ExplorerWrapper as Explorer,
} from './explorer';
Copy link
Member Author

Choose a reason for hiding this comment

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

@sgrove keeping the exports in parity here, but also exporting the inner component as we have our own plugin error boundary planned

Copy link
Contributor

@TallTed TallTed left a comment

Choose a reason for hiding this comment

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

just fixing language in a comment

}
return parse(
text,
// Tell graphql to not bother track locations when parsing, we don't need
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Tell graphql to not bother track locations when parsing, we don't need
// Tell graphql to not bother tracking locations when parsing; we don't need

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, Ted!

I'm trying to change as little as possible from the original project in this PR, but this change makes sense of course

"version": "0.9.1",
"module": "dist/index.js",
"types": "dist/index.d.ts",
"license": "MIT",
Copy link
Member Author

@acao acao Jul 28, 2023

Choose a reason for hiding this comment

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

some other thoughts about the package @sgrove & @dwwoelfel :

  • we can include a CJS export as well for backwards compatibility
  • we can import and triage existing issues and PRs
  • when we move to stylesheets and css variables, we would make it 0.11, and continue incrementing minor versions for breaking changes as such

@acao acao force-pushed the rehome-graphiql-explorer branch from 59351c0 to af305bd Compare July 30, 2023 11:06
@codecov
Copy link

codecov bot commented Jul 30, 2023

Codecov Report

Merging #3374 (2340812) into main (2348641) will increase coverage by 0.07%.
Report is 19 commits behind head on main.
The diff coverage is 78.04%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3374      +/-   ##
==========================================
+ Coverage   55.75%   55.82%   +0.07%     
==========================================
  Files         110      110              
  Lines        5243     5261      +18     
  Branches     1426     1434       +8     
==========================================
+ Hits         2923     2937      +14     
- Misses       1897     1900       +3     
- Partials      423      424       +1     
Files Coverage Δ
...raphql-language-service-server/src/GraphQLCache.ts 50.88% <50.00%> (ø)
...ql-language-service-server/src/MessageProcessor.ts 46.11% <50.00%> (-0.11%) ⬇️
...nguage-service/src/utils/getVariablesJSONSchema.ts 94.59% <98.38%> (+0.71%) ⬆️
packages/graphiql-react/src/editor/hooks.ts 38.16% <6.25%> (-0.42%) ⬇️

@acao acao force-pushed the rehome-graphiql-explorer branch 3 times, most recently from 9b38e22 to f8cfaf0 Compare July 30, 2023 16:48
@acao acao force-pushed the rehome-graphiql-explorer branch from f8cfaf0 to 2340812 Compare October 31, 2023 16:21
@rubendinho
Copy link

Hi @acao is anything blocking this / any way to help get this merged in?

Copy link
Contributor

@TallTed TallTed left a comment

Choose a reason for hiding this comment

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

minor, human-facing


### Customizing colors

The Explorer accepts a `colors` prop as a map of the class names in GraphiQL's css to hex colors. If you've edited the GraphiQL class names that control colors (e.g. `cm-def`, `cm-variable`, `cm-string`, etc.) use those same colors in the colors map. The naming of the keys in the colors map tries to align closely with the names of the class names in GraphiQL's css (note that the Explorer can't just apply the classes because of conflicts with how the css file styles inputs).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The Explorer accepts a `colors` prop as a map of the class names in GraphiQL's css to hex colors. If you've edited the GraphiQL class names that control colors (e.g. `cm-def`, `cm-variable`, `cm-string`, etc.) use those same colors in the colors map. The naming of the keys in the colors map tries to align closely with the names of the class names in GraphiQL's css (note that the Explorer can't just apply the classes because of conflicts with how the css file styles inputs).
The Explorer accepts a `colors` prop as a map of the class names in GraphiQL's css to hex colors. If you've edited the GraphiQL class names that control colors (e.g. `cm-def`, `cm-variable`, `cm-string`, etc.), use those same colors in the colors map. The naming of the keys in the colors map tries to align closely with the class names in GraphiQL's css (note that the Explorer can't just apply the classes because of conflicts with how the css file styles inputs).

1. Converted to Typescript
2. Split into multiple files
3. new `ExplorerInner` and `GraphiQLExplorerInnerProps` exports which remove some of the presentation complexity for the graphiql plugin.
4. uses modern targets and only exports esm build, does not export cjs, bundle and transpile as needed
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
4. uses modern targets and only exports esm build, does not export cjs, bundle and transpile as needed
4. uses modern targets and only exports esm build; does not export cjs; bundle and transpile as needed


## Created by OneGraph

[OneGraph](https://www.onegraph.com) provides easy, consistent access to the APIs that underlie your business--all through the power of GraphQL.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[OneGraph](https://www.onegraph.com) provides easy, consistent access to the APIs that underlie your business--all through the power of GraphQL.
[OneGraph](https://www.onegraph.com) provides easy, consistent access to the APIs that underlie your businessall through the power of GraphQL.


### Customizing arrows and checkboxes

The explorer accepts props for setting custom checkboxes (for leaf fields) and arrows (for object fields).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The explorer accepts props for setting custom checkboxes (for leaf fields) and arrows (for object fields).
The explorer accepts properties for setting custom checkboxes (for leaf fields) and arrows (for object fields).


The explorer accepts props for setting custom checkboxes (for leaf fields) and arrows (for object fields).

The props are `arrowOpen`, `arrowClosed`, `checkboxChecked`, and `checkboxUnchecked`. You can pass any react node for those props.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The props are `arrowOpen`, `arrowClosed`, `checkboxChecked`, and `checkboxUnchecked`. You can pass any react node for those props.
The properties are `arrowOpen`, `arrowClosed`, `checkboxChecked`, and `checkboxUnchecked`. You can pass any react node for those props.


You can modify the styles for the buttons that allow you to create new operations.

Pass the `styles` prop when you create the component. It's an object with two keys, `explorerActionsStyle` and `buttonStyle`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Pass the `styles` prop when you create the component. It's an object with two keys, `explorerActionsStyle` and `buttonStyle`.
Pass the `styles` property when you create the component. It's an object with two keys, `explorerActionsStyle` and `buttonStyle`.

schema?: null | GraphQLSchema;
onEdit: (edit: string) => void;
/**
* the same prop as GraphiQLProps.getDefaultFieldNames
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* the same prop as GraphiQLProps.getDefaultFieldNames
* the same property as GraphiQLProps.getDefaultFieldNames

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

Successfully merging this pull request may close these issues.

3 participants