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

[v3] migrate graphiql to vite and react compiler #3826

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

Conversation

dimaMachina
Copy link
Collaborator

  • I took my changes from GraphiQL v4 GraphiQL v4 #3685
  • added prepublishOnly with creating "graphiql.js", "graphiql.min.js", "graphiql.min.js.map", "graphiql.css", "graphiql.min.css"

Copy link
Contributor

github-actions bot commented Dec 14, 2024

The latest changes of this PR are available as canary in npm (based on the declared changesets):

@dimaMachina dimaMachina marked this pull request as ready for review December 14, 2024 02:40
@dimaMachina dimaMachina changed the title [wip] migrate graphiql to vite and react compiler [4/3] [v3] migrate graphiql to vite and react compiler Dec 14, 2024
Copy link

codecov bot commented Dec 14, 2024

Codecov Report

Attention: Patch coverage is 42.85714% with 4 lines in your changes missing coverage. Please review.

Project coverage is 63.96%. Comparing base (2a9a77c) to head (ad75cda).

Files with missing lines Patch % Lines
packages/graphiql/test/schema.js 42.85% 4 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3826      +/-   ##
==========================================
- Coverage   63.98%   63.96%   -0.03%     
==========================================
  Files          35       35              
  Lines        3088     3086       -2     
  Branches      937      949      +12     
==========================================
- Hits         1976     1974       -2     
  Misses       1107     1107              
  Partials        5        5              
Files with missing lines Coverage Δ
packages/graphiql/test/schema.js 36.76% <42.85%> (-1.81%) ⬇️

@dimaMachina dimaMachina changed the title [4/3] [v3] migrate graphiql to vite and react compiler [2/2] [v3] migrate graphiql to vite and react compiler Dec 14, 2024
Base automatically changed from react-compiler to main December 14, 2024 13:31
@dimaMachina dimaMachina changed the title [2/2] [v3] migrate graphiql to vite and react compiler [v3] migrate graphiql to vite and react compiler Dec 14, 2024
crossorigin
src="https://unpkg.com/react-dom@18/umd/react-dom.production.min.js"
></script>
<script src="/dist/index.umd.js"></script>
Copy link
Member

Choose a reason for hiding this comment

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

this should be graphiql.js or else it will break all the unpkg implementations

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

what do you mean by it will break all the unpkg implementations?

/dist/index.umd.js === /graphql.js
/dist/index.umd.js === /graphql.min.js

app.use(express.static(path.join(__dirname, '..')));
} else {
app.get('/', (req, res) => {
res.redirect('http://localhost:5173');
Copy link
Member

Choose a reason for hiding this comment

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

this is fine as long as it's serving the unminified umd version

Copy link
Collaborator Author

@dimaMachina dimaMachina Dec 15, 2024

Choose a reason for hiding this comment

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

I made to use umd build only on CI, and you can see that all tests pass

locally I replace umd import

<!--vite-replace-start-->
<script
crossorigin
src="https://unpkg.com/react@18/umd/react.production.min.js"
></script>
<script
crossorigin
src="https://unpkg.com/react-dom@18/umd/react-dom.production.min.js"
></script>
<script src="/dist/index.umd.js"></script>
<link href="/dist/style.css" rel="stylesheet" />
<!--vite-replace-end-->

with src/cdn.ts

const htmlForVite = /* HTML */ `
<script type="module">
import React from 'react';
import ReactDOM from 'react-dom/client';
import GraphiQL from './src/cdn';
Object.assign(globalThis, { React, ReactDOM, GraphiQL });
</script>
<link href="/src/style.css" rel="stylesheet" />
`;

@acao
Copy link
Member

acao commented Dec 14, 2024

this looks good so far, but we still need graphiql.min.js and graphiql.js as the UMD versions or it will break all the many unpkg/jsdelivr implementations, just do a github code search and you'll see thousands. the benefits of consistent naming here are much less important than this pivotal backwards compatibility for our majority of users.

"exports": {
"./package.json": "./package.json",
"./style.css": "./dist/style.css",
"./graphiql.css": "./dist/style.css",
Copy link
Member

@acao acao Dec 14, 2024

Choose a reason for hiding this comment

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

exports don't work with jsdelivr/unpkg unfortunately, we must publish to npm with these exact files

Copy link
Collaborator Author

@dimaMachina dimaMachina Dec 15, 2024

Choose a reason for hiding this comment

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

this is for backward compatibility since I added exports field

I added prepublishOnly with the exact files

"prepublishOnly": "cp dist/index.umd.js graphiql.js && cp dist/index.umd.js.map graphiql.js.map && cp dist/index.umd.js graphiql.min.js && cp dist/index.umd.js.map graphiql.min.js.map && cp dist/style.css graphiql.css && cp dist/style.css graphiql.min.css",

@dimaMachina
Copy link
Collaborator Author

this looks good so far, but we still need graphiql.min.js and graphiql.js as the UMD versions or it will break all the many unpkg/jsdelivr implementations, just do a github code search and you'll see thousands. the benefits of consistent naming here are much less important than this pivotal backwards compatibility for our majority of users.

see #3826 (comment)

@dimaMachina dimaMachina requested a review from acao December 15, 2024 15:15
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.

2 participants