Fix broken GraphiQL minified build #982
Conversation
The builds across the packages currently depend on the transpiled files to be in es5 (for example uglify is used to minify graphiql and it doesn't support es6).
The source for the minify operation should be graphiql.js for it to work properly.
Codecov Report
@@ Coverage Diff @@
## master #982 +/- ##
=======================================
Coverage 43.14% 43.14%
=======================================
Files 64 64
Lines 2934 2934
Branches 635 635
=======================================
Hits 1266 1266
Misses 1400 1400
Partials 268 268Continue to review full report at Codecov.
|
|
Babel doesn't because it only transpiles from the src to dist and didn't create the bundle. The bundle is created by browserify after babel transpiles |
|
Babelify should help in this case |
To ensure babelify transforms files in node_modules.
|
Adding the browserify transform config for babelify works for bundling graphiql without changing the target from esnext. This however means that all the browserify operations (currently bundling to graphiql.js and minifying to graphiql.min.js) would apply the babelify transform as well, but this shouldn't be a problem and works at the moment. |
|
The reason why this is the solution I am proposing is this: https://github.com/babel/babelify#why-arent-files-in-node_modules-being-transformed Since the problematic package is gls-interface (which is in the node_modules), the browserify transform wouldn't transpile those by default. |
|
this is why i tried for weeks to switch to rollup earlier in the year. i
think webpack is a good solution. thanks for looking into this. not sure
how this regressed, we must have broken the build scripts
…On Sat, Oct 19, 2019 at 5:45 AM Samuel ***@***.***> wrote:
The reason why this is the solution I am proposing is this:
https://github.com/babel/babelify#why-arent-files-in-node_modules-being-transformed
Since the problematic package is gls-interface (which is in the
node_modules), the browserify transform wouldn't transpile those by default.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#982?email_source=notifications&email_token=AAKOFF7ZIEILPLYFYWR4TVTQPLJK3A5CNFSM4JCM2FQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBXKEVQ#issuecomment-544121430>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKOFF7FOCQRJMUX66SSWFDQPLJK3ANCNFSM4JCM2FQQ>
.
|
|
Yeah, not a big fan of browserify myself 😅 If there are no other concerns, perhaps you can take a look at this PR locally to verify the implementation. For me, it seems to work fine. |
|
thanks so much @imolorhe !! going to cut another release of graphiql here now |
|
@acao Was this fix supposed to be part or version 0.16? Seems to still be broken for me. Only the non-minified version is working. |
|
hmmm! well I have a PR (the huge bundle one) with passing tests that
definitely fixes that, should be ready to merge unless there are new
upstream conflicts from today.
the new netlify build in that PR directly uses the min.js, so you can
preview it for yourself. terribly sorry for the trouble, i had manually
confirmed it last release i’d thought. is this from npm?
…On Thu, Nov 7, 2019 at 8:22 AM jbblanchet ***@***.***> wrote:
@acao <https://github.com/acao> Was this fix supposed to be part or
version 0.16? Seems to still be broken for me. Only the non-minified
version is working.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#982?email_source=notifications&email_token=AAKOFF4ZTDFV5NPOKH5C3F3QSQQBTA5CNFSM4JCM2FQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDMR3AI#issuecomment-551099777>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKOFF44FCK3GIIQNZGFPOTQSQQBTANCNFSM4JCM2FQQ>
.
|
|
From jsdelivr. Switching over from cdnjs since it's been dead for a month. Using |
|
@jbblanchet i'll see if I can't kick out a quick fix for this tonight, but otherwise the existing PR #973 will address this. terribly sorry. the netlify preview build and e2e tests as of this new PR #973 will use the minified bundle as well. I had set them to use graphiql.js for debugging :( my bad! |
|
@acao No worries, it happens. Thanks for taking care of it, appreciate the hard work. |
The current source used for minifying graphiql in the build script is wrong. Therefore the build is broken. After debugging for the reason while the build was failing, I figured out that in order to use the correct source for the build, we need all the packages used in the build to be transpiled to their es5 equivalents for the build process to work. Main reason being that uglify doesn't support es6 syntax and requires es5 to minify the file. Given that the tsconfig specified the target as
esnext, the builds were transpiled to their es6/es7 equivalents, which is required by graphiql as a dependency, and so uglify breaks.I'm not sure why we decided to set the tsconfig to
esnext, but this question should be answered before merging this PR.This closes #976