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

graphql: upgrade UI to v2 #27294

Merged
merged 5 commits into from
May 22, 2023
Merged

graphql: upgrade UI to v2 #27294

merged 5 commits into from
May 22, 2023

Conversation

s1na
Copy link
Contributor

@s1na s1na commented May 17, 2023

Upgrades graphiql to v2.4.4. The interface has become much nicer. There are extra features like tabs, history, dark mode etc.

Also now instead of sourcing the JS and CSS files from CDN they are embedded in the binary. That makes offline usage of the UI possible.

It works fine. Only there is a slight issue: sometimes white spaces are displayed weird
Screenshot 2023-05-17 at 13 53 31

I realized that when in index.html I replace this line:

<script src="/graphql/ui/graphiql.min.js" type="application/javascript"></script>

with:

<script src="https://unpkg.com/graphiql/graphiql.min.js" type="application/javascript"></script>

the issue goes away. Even tho they are the same file!

@s1na s1na marked this pull request as ready for review May 17, 2023 20:34
@s1na
Copy link
Contributor Author

s1na commented May 17, 2023

Turns out adding: <meta charset="UTF-8"> fixes the issue. Read for review.

@holiman
Copy link
Contributor

holiman commented May 19, 2023

This PR loads the data into memory,

WARN [05-19|09:10:55.730] Loaded graphiql asset                    asset=index.html size=1144
WARN [05-19|09:10:55.735] Loaded graphiql asset                    asset=graphiql.min.css size=411,956
WARN [05-19|09:10:55.744] Loaded graphiql asset                    asset=graphiql.min.js  size=980,555
WARN [05-19|09:10:55.744] Loaded graphiql asset                    asset=react.production.min.js size=11440
WARN [05-19|09:10:55.744] Loaded graphiql asset                    asset=react-dom.production.min.js size=120,585

Is it worth it? Browsers should only fetch it once, and then cache it for the future. But even if that is not the case, I tested fetching the heaviest file 100 times

real	0m1.122s
user	0m0.395s
sys	0m0.626s

With on-the-fly loading:

real	0m1.487s
user	0m0.368s
sys	0m0.631s

I'd say it's about same same. If anyone wants to make a public graphql server able to handle high demands, they should add a caching nginx or something in front. I'll push some changes...

@holiman
Copy link
Contributor

holiman commented May 19, 2023

PTAL. I changed it so it now loads on demand. Also, I undid the charset-fix, so we can keep the asset identical to the upstream version. The proper fix is to set the charset in the http header instead.

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

@vinioleg2015zem vinioleg2015zem linked an issue May 21, 2023 that may be closed by this pull request
@s1na
Copy link
Contributor Author

s1na commented May 22, 2023

PTAL. I changed it so it now loads on demand. Also, I undid the charset-fix, so we can keep the asset identical to the upstream version. The proper fix is to set the charset in the http header instead.

Good point. We don't need to impose the initial loading on every user even if they don't want to use graphiql. Your changes LGTM

@holiman holiman added this to the 1.11.7 milestone May 22, 2023
@holiman holiman merged commit b46d37e into ethereum:master May 22, 2023
1 of 2 checks passed
antonydenyer pushed a commit to antonydenyer/go-ethereum that referenced this pull request Jul 28, 2023
Upgrades  graphiql to v2.4.4. The interface has become much nicer, and there are extra features like tabs, history, dark mode etc.

This change also now uses golang embed to bundle the resources.

---------

Co-authored-by: Martin Holst Swende <martin@swende.se>
MoonShiesty pushed a commit to MoonShiesty/go-ethereum that referenced this pull request Aug 30, 2023
Upgrades  graphiql to v2.4.4. The interface has become much nicer, and there are extra features like tabs, history, dark mode etc.

This change also now uses golang embed to bundle the resources.

---------

Co-authored-by: Martin Holst Swende <martin@swende.se>
devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
Upgrades  graphiql to v2.4.4. The interface has become much nicer, and there are extra features like tabs, history, dark mode etc.

This change also now uses golang embed to bundle the resources.

---------

Co-authored-by: Martin Holst Swende <martin@swende.se>
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 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.

> content://media/external/downloads/1000000694
2 participants