RFC: Teach compiler `--persist` option #1846

wincent · 2017-05-31T18:44:35Z

Sample usage:

relay-compiler --src ./src --schema schema.graphql --persist scripts/persistQuery.js

Where persistQuery.js looks like this:

function persistQuery(text) {
  var id = someLogicToPersistQueryText(text);
  return Promise.resolve(id);
}

The alternative in the absence of this option is to copy-paste the entire RelayCompilerBin implementation just to override that bit.

Note that webpack makes this ghastly — necessitating the use of eval — because it wants to rewrite the require of the passed in module, which of course fails because it is dynamic. Google search turns up a bunch of results like this one lamenting the situation:

https://stackoverflow.com/questions/30575060/require-js-files-dynamically-on-runtime-using-webpack

Apparently it's better in webpack 2.

wincent · 2017-05-31T18:45:33Z

This is ugly as hell, but throwing it up there as an RFC anyway. In my own side-project, I went down the path of forking RelayCompilerBin, but it seems pretty limiting not to be able to use the relay-compiler executable for anything but very simple use cases.

wincent · 2017-05-31T18:46:13Z

packages/relay-compiler/bin/RelayCompilerBin.js

+  // Using eval here to prevent webpack from trying to rewrite the require and
+  // failing.
+  const persistQuery = persistModule &&
+    eval(`require(${JSON.stringify(persistModule)})`);


Isn't there something like require.actual()? Or maybe alias require to something else?

In vanilla node, there is not (that's a Jest thing). I looked on the interwebs, but I couldn't find anything. Moving to webpack 2 would provide us with some more options, I think, but that's obviously a bigger change.

What about aliasing require so webpack doesn't transform it? Or shell out to a separate process?

Good idea. Let me see if aliasing works.

Seems like it does work (at least, build runs without blowing up); webpack lets the line in question through as:

var persistQuery = persistModule && global['require'](persistModule);

in the built output.

josephsavona · 2017-05-31T21:43:11Z

I was going to send a PR for this too, definitely something we should add.

wincent · 2017-06-01T06:28:44Z

Gonna import this to run Flow tests internally (my local set-up is borked right now).

wincent · 2017-06-01T06:29:41Z

Gonna import this to run Flow tests internally (my local set-up is borked right now).

Heh, or I could just look at Travis...

kassens · 2017-06-01T16:14:55Z

Webpack actually aliases global, I also couldn't find a way to do this and resorted to eval in a similar situation.
Do you have a pointer how this works in webpack 2? It might help in my situation.

wincent · 2017-06-01T16:22:39Z

Webpack actually aliases global, I also couldn't find a way to do this and resorted to eval in a similar situation.

I was hopeful that this would work because the built output looks like this:

            var persistModule = options.persist && __webpack_require__(5).resolve(process.cwd(), options.persist);
            if (persistModule && !__webpack_require__(6).existsSync(persistModule)) {
              throw new Error('--persist path does not exist: ' + persistModule + '.');
            }
            // Need to hide the `require` here to prevent webpack from rewriting
            // it and failing.
            var persistQuery = persistModule && global['require'](persistModule);

Note how it hasn't mangled global['require']. I also couldn't see any direct messing with global, at least inside the relay-compiler bin file, although be getting messed with somewhere else. I haven't tested this separately yet, but I will to confirm.

Do you have a pointer how this works in webpack 2? It might help in my situation.

(Whoops submitted too soon).

I just found a few mentions in my Google searches to "dynamic requires" not being enabled by default in v2. (Example).

robrichard · 2017-06-04T14:43:08Z

Do you need to run the relay-compiler bin through webpack at all? Since it's a node command line script, couldn't you leave all the requires intact?

wincent · 2017-06-06T16:49:24Z

Cross-referencing: #1710 (review)

@robrichard:

Do you need to run the relay-compiler bin through webpack at all? Since it's a node command line script, couldn't you leave all the requires intact?

True. My recollection is that we're doing this as an optimization to minimize cold-start time.

Sample usage: relay-compiler --src ./src --schema schema.graphql --persist scripts/persistQuery.js Where `persistQuery.js` looks like this: ``` function persistQuery(text) { var id = someLogicToPersistQueryText(text); return Promise.resolve(id); } ``` The alternative in the absence of this option is to copy-paste the entire `RelayCompilerBin` implementation just to override that bit. Note that webpack makes this ghastly — necessitating the use of `eval` — because it wants to rewrite the `require` of the passed in module, which of course fails because it is dynamic. Google search turns up a bunch of results like this one lamenting the situation: https://stackoverflow.com/questions/30575060/require-js-files-dynamically-on-runtime-using-webpack Apparently it's better in webpack 2.

This time actually tested it and confirmed that it works.

josephsavona · 2017-06-13T02:09:35Z

Ship it!

facebook-github-bot · 2017-06-17T00:13:28Z

@wincent has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

josephsavona · 2017-08-14T16:13:48Z

@unirey No idea. cc @leebyron

wincent · 2017-08-14T22:23:22Z

I imported it but it has since grown stale. I need to dust it off and rebase it. A question was raised into whether the change as I had made it is would work everywhere, and I haven't had time to (re-)validate that myself yet.

wincent · 2017-08-22T17:56:18Z

I would also prefer not take a slippery slope of creating, maintaining my own version of Relay.

@unirey: not to dismiss your point (I'd like to see this option merged, obviously), but note that it's not really creating your own version of Relay: you just need to fork a single file (and in fact, internally at Facebook we do exactly that: we have a custom "compiler" executable that is configured to suit our specific needs). I did this for a side-project of mine, for instance: compile.js.

robrichard · 2017-08-22T18:10:32Z

@wincent forking that file using a version of relay newer than 1.0.0 requires formatGeneratedModule which is not publicly exported. See #1927

wincent · 2017-08-22T18:16:23Z

@wincent forking that file using a version of relay newer than 1.0.0 requires formatGeneratedModule which is not publicly exported.

I think it's probably reasonable to expose that, although we've historically been very conservative and minimal in terms of what we include in the public API. The main question is whether this stuff should be exposed at the relay-compiler level, or rather just the building blocks for it lower down, in the graphql-compiler.

Summary: Fixes #1927 wincent This will export `formatGeneratedModule` as discussed in #1846 (comment). I exported it in `relay-compiler` (as opposed to `graphql-compiler`) since that is where it is currently located. Closes #2054 Reviewed By: kassens Differential Revision: D5690522 Pulled By: wincent fbshipit-source-id: bc7fd24152d1a1f62e28432fe3e7e1d6b7fef158

GrigoryPtashko · 2017-09-17T13:34:03Z

Hello, everyone. Is this going to be merged actually? That's a nice option I'd like to use instead of forking..

schickling · 2017-10-03T15:47:14Z

yusinto · 2018-01-17T09:40:40Z

I published my own package relay-compiler-plus which supports persisted queries and compiles graphql-js schema as well. Hope that helps for now.

jwb · 2018-01-26T18:30:26Z

It would be really great to have support from the core to proceed with this!

sibelius · 2018-01-26T18:39:32Z

@jstejada any plans for this feature?

devknoll · 2018-01-26T19:28:56Z

I haven't spoken to the team yet, but my hunch is that if someone wanted to update this diff to make persist a command that is executed, rather than a JS module, we might be willing to merge it.

sibelius · 2018-02-19T14:26:09Z

@yusinto can u send a PR making persist a command?

yusinto · 2018-02-19T21:40:12Z

Yes I will submit a pr soon. Thanks for the suggestion @sibelius

yusinto · 2018-02-28T13:44:47Z

Done. Here it is.

alloy · 2018-02-28T14:51:10Z

To avoid confusion, I’ll close this PR for now in favour of #2354.

facebook-github-bot added the CLA Signed label May 31, 2017

wincent changed the title ~~Teach compiler --persist option~~ RFC: Teach compiler --persist option May 31, 2017

wincent commented May 31, 2017

View reviewed changes

wincent added 3 commits June 8, 2017 22:51

Hide require access using global rather than eval

4a62af4

Use __non_webpack_require__

d6848ee

This time actually tested it and confirmed that it works.

johntran mentioned this pull request Aug 2, 2017

[modern] Persisted GraphQL Queries in Modern? #2008

Open

robrichard mentioned this pull request Aug 22, 2017

expose formatGeneratedModule publicly #2054

Closed

adjourn mentioned this pull request Dec 23, 2017

[Modern] Client schema docs and examples #1656

Closed

alloy mentioned this pull request Feb 28, 2018

Persisted queries #2354

Closed

alloy closed this Feb 28, 2018

alloy mentioned this pull request Aug 21, 2018

[RFC] Allow for more complex configuration of relay-compiler / babel plugin #2518

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Teach compiler `--persist` option #1846

RFC: Teach compiler `--persist` option #1846

wincent commented May 31, 2017

wincent commented May 31, 2017

wincent May 31, 2017

josephsavona May 31, 2017

wincent May 31, 2017

josephsavona Jun 1, 2017

wincent Jun 1, 2017

wincent Jun 1, 2017

josephsavona commented May 31, 2017

wincent commented Jun 1, 2017

wincent commented Jun 1, 2017

kassens commented Jun 1, 2017

wincent commented Jun 1, 2017 •

edited

Loading

robrichard commented Jun 4, 2017

wincent commented Jun 6, 2017

josephsavona commented Jun 13, 2017

facebook-github-bot commented Jun 17, 2017

josephsavona commented Aug 14, 2017 •

edited

Loading

wincent commented Aug 14, 2017

wincent commented Aug 22, 2017

robrichard commented Aug 22, 2017

wincent commented Aug 22, 2017

GrigoryPtashko commented Sep 17, 2017

schickling commented Oct 3, 2017

yusinto commented Jan 17, 2018

jwb commented Jan 26, 2018

sibelius commented Jan 26, 2018

devknoll commented Jan 26, 2018

sibelius commented Feb 19, 2018

yusinto commented Feb 19, 2018

yusinto commented Feb 28, 2018

alloy commented Feb 28, 2018 •

edited

Loading

RFC: Teach compiler --persist option #1846

RFC: Teach compiler --persist option #1846

Conversation

wincent commented May 31, 2017

wincent commented May 31, 2017

wincent May 31, 2017

Choose a reason for hiding this comment

josephsavona May 31, 2017

Choose a reason for hiding this comment

wincent May 31, 2017

Choose a reason for hiding this comment

josephsavona Jun 1, 2017

Choose a reason for hiding this comment

wincent Jun 1, 2017

Choose a reason for hiding this comment

wincent Jun 1, 2017

Choose a reason for hiding this comment

josephsavona commented May 31, 2017

wincent commented Jun 1, 2017

wincent commented Jun 1, 2017

kassens commented Jun 1, 2017

wincent commented Jun 1, 2017 • edited Loading

robrichard commented Jun 4, 2017

wincent commented Jun 6, 2017

josephsavona commented Jun 13, 2017

facebook-github-bot commented Jun 17, 2017

josephsavona commented Aug 14, 2017 • edited Loading

wincent commented Aug 14, 2017

wincent commented Aug 22, 2017

robrichard commented Aug 22, 2017

wincent commented Aug 22, 2017

GrigoryPtashko commented Sep 17, 2017

schickling commented Oct 3, 2017

yusinto commented Jan 17, 2018

jwb commented Jan 26, 2018

sibelius commented Jan 26, 2018

devknoll commented Jan 26, 2018

sibelius commented Feb 19, 2018

yusinto commented Feb 19, 2018

yusinto commented Feb 28, 2018

alloy commented Feb 28, 2018 • edited Loading

RFC: Teach compiler `--persist` option #1846

RFC: Teach compiler `--persist` option #1846

wincent commented Jun 1, 2017 •

edited

Loading

josephsavona commented Aug 14, 2017 •

edited

Loading

alloy commented Feb 28, 2018 •

edited

Loading