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

Broken minified build for graphiql 0.16.0 #976

Closed
dinvlad opened this issue Oct 7, 2019 · 8 comments · Fixed by #982
Closed

Broken minified build for graphiql 0.16.0 #976

dinvlad opened this issue Oct 7, 2019 · 8 comments · Fixed by #982

Comments

@dinvlad
Copy link

dinvlad commented Oct 7, 2019

Hi All,

The jsDelivr CDN distribution of GraphiQL seems broken at the moment.

I followed the official example at https://github.com/graphql/graphiql/blob/master/packages/examples/graphiql-cdn/index.html, and then changed the CDN URL from ./graphiql.js to https://cdn.jsdelivr.net/npm/graphiql/graphiql.min.js, which results in Minified React error #130.

Changing it to the full version ( https://cdn.jsdelivr.net/npm/graphiql/graphiql.js) seems to fix it.

Not sure if this issue belongs here, just something to note.
Thanks!

@acao
Copy link
Member

acao commented Oct 9, 2019

@dinvlad thanks for reporting this, i’ll take a look, the latest release builds with typescript so its a big factor here

@imolorhe
Copy link
Contributor

@acao Maybe this line needs to be changed to:

+ browserify -g browserify-shim -t uglifyify -s GraphiQL graphiql.min.js | uglifyjs -c > graphiql.min.js
- browserify -g browserify-shim -t uglifyify -s GraphiQL graphiql.js | uglifyjs -c > graphiql.min.js

if the intention is to use the file that was just built in the previous line to create the minified version?

@imolorhe
Copy link
Contributor

Created a PR to address this issue. @acao

@acao
Copy link
Member

acao commented Oct 18, 2019 via email

@acao acao closed this as completed in #982 Oct 19, 2019
@acao acao reopened this Nov 17, 2019
@acao
Copy link
Member

acao commented Nov 17, 2019

@imolorhe apologies, did not mean to close this issue!

we will track this known bug here.

fix is a few PRs away.

@acao acao changed the title Minified jsDelivr file is broken Minified build for graphiql 0.16.0 Nov 17, 2019
@acao acao changed the title Minified build for graphiql 0.16.0 Broken minified build for graphiql 0.16.0 Nov 17, 2019
acao added a commit that referenced this issue Nov 18, 2019
acao added a commit that referenced this issue Nov 18, 2019
acao added a commit that referenced this issue Nov 18, 2019
acao added a commit that referenced this issue Nov 18, 2019
acao added a commit that referenced this issue Nov 18, 2019
acao added a commit that referenced this issue Nov 19, 2019
acao added a commit that referenced this issue Nov 20, 2019
acao added a commit that referenced this issue Nov 20, 2019
acao added a commit that referenced this issue Nov 20, 2019
acao added a commit that referenced this issue Nov 20, 2019
acao added a commit that referenced this issue Nov 23, 2019
@acao
Copy link
Member

acao commented Nov 23, 2019

unfortunately we won't have anyone to review PRs this weekend or for a while, so despite having a working minified bundle demo minified bundle it may still be a few weeks until a maintainer has the time to review this. But hey once it's released it'll be less than 1/4 of the original bundle size!

i spent a few nights just trying to get the legacy browserify build to work, but for whatever reason I couldn't, so the only option was the full path forward with webpack (I had spiked with Rollup previously some months ago, and I just couldnt get the legacy umd global interface working right). Webpack is the most versatile tool for the job IMO.

I originally introduced it as a mega-PR but then tried to slice it into smaller chunks, so i broke the PR up into several. It wasn't possible to break them down much further and still having building PRs. This is just unfortunately a chokepoint, given the complexity, so any peer review folks want to help with would be excellent and much appreciated, and would help my co-maintainers be able to review much more quickly.

@dinvlad
Copy link
Author

dinvlad commented Nov 23, 2019

Sounds good, thanks for working on this!!

@acao
Copy link
Member

acao commented Nov 26, 2019

https://unpkg.com/browse/graphiql@0.17.0/graphiql.min.js

0.17.0 minified build is now available. generated using webpack. re-creating it requires two pending and already well reviewed PRs to be merged, but it's been thoroughly tested on chrome, firefox, safari.

an alpha probably would have been a better choice but some would argue 0.x.y versions are always prerelease.

either way, there you have it! please let me know if there are any regressions. i have not tested this new webpack generated minified build in IE11 yet, which is another issue. we are adding new maintainers now so we should be able to start getting more things merged.

also noting that 0.14.2 min.js was 1.15mb, and it's now 744kb, so my math was wrong, but still a significant improvement! haha

@acao acao closed this as completed Dec 1, 2019
acao added a commit that referenced this issue Dec 3, 2019
* esm for all packages
* convert browserify to webpack, fixes #976
* remove vendor files for react/etc
* fix webpack targets for minified build
* add bundle step to netlify
* use cross-env for every script that uses env
* fix markdown-it bug by force-adding the entities version
* simpler `cypress-open` script

Co-Authored-By: Benjie Gillam <benjie@jemjie.com>
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 a pull request may close this issue.

3 participants