Graphiql: Cannot build with webpack

[wmertens]

I'm trying to bundle graphiql which uses codemirror-graphql which uses graphql-language-service.

The build is failing because webpack can't parse the bundled js.flow files, and it is trying to parse those because of the dynamic import at https://github.com/graphql/graphql-language-service/blob/master/packages/graphql-language-service-interface/src/GraphQLLanguageService.js#L87 (probably) which could be importing anything at all.

Is it not possible to limit the require by making it something like:

require(`./${somePath}.js`)

?

If not, is it possible to not include the flow js files in the dist folder?

The webpack error is:

Module parse failed: .../node_modules/graphql-language-service-interface/dist/getOutline.js.flow Unexpected token (11:12)
You may need an appropriate loader to handle this file type.
|  */
|
| import type {
|   Outline,
|   TextToken,
 @ ./node_modules/graphql-language-service-interface/dist ^.*$
 @ ./node_modules/graphql-language-service-interface/dist/GraphQLLanguageService.js
 @ ./node_modules/graphql-language-service-interface/dist/index.js
 @ ./node_modules/codemirror-graphql/lint.js
 @ ./node_modules/graphiql/dist/components/QueryEditor.js
 @ ./node_modules/graphiql/dist/components/GraphiQL.js
 @ ./node_modules/graphiql/dist/index.js
 @ ./lib/universal/Graphql/index.jsx
 @ ./lib/universal/Root.jsx
 @ ./lib/client/index.js
 @ multi babel-polyfill stratokit/client

[wmertens]

For now I worked around it by using this as a webpack plugin:

            new webpack.ContextReplacementPlugin(
                /graphql-language-service-interface[\\/]dist$/,
                new RegExp(`^\\./.*\\.js$`)
            )

but of course this is far from ideal. Isn't there a better approach than this dynamic require?

[wmertens]

I just realized that the above webpack output is just a warning and in fact my build was failing for another reason 🙄. In any case, it's still kind of an issue, no? 😅

[ryanbahniuk]

This warning is also throwing for me and I'd love to find a way to prevent it. I tried stripping flow tags through a babel plugin in my package, but that did not work :-(

[wmertens]

@ryanbahniuk, try the workaround I posted?

[asiandrummer]

@wmertens thanks for the report! Let me check this out and see what the problem is

[soosap]

Having the same error. I can confirm that the workaround posted by @wmertens prevents the error/warning. But yes, it's a workaround, would be nice if it gets fixed :)

[asiandrummer]

@soosap @wmertens I'm onto fixing this issue! It's just taking a bit of time for me to get to this - I promise to deal with it before this weekend. Sorry for the holdup!

[wmertens]

No stress, thanks for looking at it :)

On Wed, Aug 2, 2017 at 12:08 AM Hyo Jeong [e-mail] wrote:

@soosap https://github.com/soosap @wmertens
https://github.com/wmertens I'm onto fixing this issue! It's just
taking a bit of time for me to get to this - I promise to deal with it
before this weekend. Sorry for the holdup!

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/graphql/graphql-language-service/issues/128#issuecomment-319511397,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADWlh-5n2iBW0GE8vaH2aboyJmo3IVDks5sT6HzgaJpZM4Och1S
.

[wouter-vdb]

This is most probably related to (or a consequence of) #111

[asiandrummer]

@wmertens I tried to create my own Webpack package to include graphiql in the bundle (therefore codemirror-graphql and graphql-language-service-interface as well), and I wasn't able to reproduce this problem. I do agree with @wouter-vdb that this is probably caused by #111 and want to resolve this issue as soon as possible for you guys. Is there any other packages/bundle builds that I can try to reproduce/fix this?

FWIW I should be more attentive to this issue for a while - feel free to leave a comment and I'll check periodically in a timely manner.

[wmertens]

Aha, this issue is actually a duplicate of #111 with the same workaround I also found :)

So, the real problem is, why do you need the dynamic require? The quick fix I post above (require(`./${somePath}.js`)) should work with webpack, and maybe browserify.

Are you outputting warnings on your webpack build? I can't imagine how it would not complain…

[wouter-vdb]

This test project reproduces the issue. Hope this helps.

PS: You could use stats.hasErrors() and stats.hasWarnings() to create a (unit-)test.

[asiandrummer]

@wmertens @wouter-vdb the dynamic require was to support developers to add their own validation rules when running diagnostics. I'll make the changes that you suggested (require(./${somePath}.js)) and run my test with the test project.

Apologies for this issue lingering longer than expected - as non-webpack user I've been having a bit of trouble figuring out the problem/ways to test.

[wmertens]

Would it not make more sense to allow passing rules as an object? That way,
the dev would be doing the require from whatever location they like. Our
workaround only allows importing from that single directory…

On Thu, Aug 10, 2017, 3:11 AM Hyo Jeong [e-mail] wrote:

@wmertens https://github.com/wmertens @wouter-vdb
https://github.com/wouter-vdb the dynamic require was to support
developers to add their own validation rules when running diagnostics. I'll
make the changes that you suggested (require(./${somePath}.js)) and run
my test with the test project.

Apologies for this issue lingering longer than expected - as non-webpack
user I've been having a bit of trouble figuring out the problem/ways to
test.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/graphql/graphql-language-service/issues/128#issuecomment-321422668,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADWlnFXZ0Oza0KyM43cVHn4LMYqMrjnks5sWlikgaJpZM4Och1S
.

[ajhyndman]

Is there any way I can assist in getting a fix for this across the line?

[sw-yx]

same here. would prefer to just add a loader as they request rather than the hacky plugin...

[sw-yx]

for what its worth i tried the workaround given and it did not work :(

[jonaskello]

Having the same problem and the workaround worked for me.

[igl]

How is this still thing? geez

[acao]

@lostplan should we work to change this interface? this require(${template string}) implementation also breaks tree shaking tools such as common-shake, preventing a slimmer build of graphiql

[JoshRobertson]

Is there no way to make this a priority? This issue has been open for 2 years. We can't use this package until this issue is resolved.

[acao]

I will be providing a webpack example soon, as I haven't had an issue except with common shake. We are working on reworking it to work with tree shaking soon, might require breaking changes.

[benjie]

In case it helps, here's the webpack configuration we're currently using in PostGraphile to bundle GraphiQL:

https://github.com/graphile/postgraphile/blob/7979cfb10f3246c7c198be8db2c929e82e0e4323/scripts/make-assets#L49-L124

[acao]

Node 9.7+ supports dynamic imports which would at runtime either dynamically load the module or in webpack dynamically load the chunk. If we ever have CDN hosted plugins for the browser they could even load using this mechanism.

I wonder if its ok to drop node 8 support then?

This would get around the dynamic require() issue and i think resolve these webpack issues upstream.

const myPlugin = await import(`plugins/${myPlugin})`)

To note, for vscode, node 10.12+ is required

[acao]

Also, here's a webpack config where I followed above adviced to workaround the meddlesome issues:

These lines are to force it to only resolve dist/js for the meddlesome gls- packages, following @wmertens's example
https://github.com/acao/graphiql-1-poc-monaco/blob/master/webpack.config.js#L62

And I was sure to resolve the mjs for graphql
https://github.com/acao/graphiql-1-poc-monaco/blob/master/webpack.config.js#L33

And be sure to add mjs to the file extensions as well, but you should probably just be doing that anyways!

This doesnt use graphiql actually, in fact it uses monaco, but the webpack issues are almost exactly the same with just the LSP packages, but it's similar to @benjie's config where we use ContextReplacementPlugin and all

[acao]

Hopefully graphql-js is going to switch away from mjs to using esm.js

[acao]

@Neitsch thoughts about https://github.com/graphql/graphiql/issues/882#issuecomment-526920549? We could just do a bulk replace even for now, I think this would fix it but i could be wrong

[Neitsch]

I'm not quite sure I follow. Is the suggestion to replace require with import? Given your explanation above, it might work. However, Node 8 EOL is Dec 31st. I don't think we should drop support before that :(

Would it be possible to declare each rule that is being required as a separate entry point? That would be my understanding of how we can provide the necessary chunks.

[acao]

@Neitsch i have better idea that will work fine until the plugin system arrives, without requiring node 8
webpack just needs an extension always, for the require() arg. so for this, detect whether theres an extension on the input path, and if so, use a hash of functions to require(${path}.js) as webpack wants, or whatever extension, as a utility lib? i think we only have one case rn though but it might be nice to be able to use this elsewhere.
i can see the code but im still too exhausted from this flu to write it 😂

[acao]

A PR is ready that solves this issue if anyone is interested! #973

[acao]

A PR that fixed this issue was merged, and the code was released. We have a resolution in 0.17.0. Thanks everyone for helping us get to the bottom of this. And yes, I even had someone from the webpack team review the file utility we needed to create for explicit extensions in require() and import() etc