Language plugin support. #2293

alloy · 2018-01-21T00:18:30Z

Closes #2073.
Closes #1710.

In preparation for #2073 and as requested by @kassens, this adds language plugin support to relay-compiler.

The whole plugin aspect is really just a matter of consolidating all language specific APIs in the RelayLanguagePluginInterface.js file and moving all of the pre-existing JS/Flow related code around to conform to that plugin interface. i.e. nothing should have changed in the existing JS/Flow handling.

The one actual additions are:

making relay-runtime able to load artifacts that are ES6 modules (with default exports)
the ability to store all artifacts in a single directory (artifactDirectory), such that fragment references can easily be imported from other artifacts without the need for the Haste module system. i.e. all imports assume the imported module is a sibling of the requesting module. This should also improve the experience of JS/Flow users.

With this plugin infrastructure in place, @kastermester and I have been able to build a TypeScript plugin https://github.com/relay-tools/relay-compiler-language-typescript. (You can find example artifacts here.)

Things that are left to do:

Document all of the types in the plugin interface
Remove version changes from package.json files
Rebase on latest upstream master
Verify that before and after JS/Flow artefacts are identical

But these are not critical to being able to start reviewing and providing feedback.

Test plan

See existing JS plugin in action in this fork of the TODO example app https://github.com/alloy/relay-examples/tree/language-plugin-test/todo
See TS plugin in action in the forked TODO example app in the TS plugin repo, by following these steps https://github.com/relay-tools/relay-compiler-language-typescript#installation

Adding the TS plugin to the TODO example app would require the following:

yarn add relay-compiler-language-typescript --dev

Tell babel-plugin-relay where to find artifacts (assuming you want fragment references to be type checked) in .babelrc:

{
  "plugins": [
    ["relay", { "artifactDirectory": "./ts/__relay_artifacts__" }],
    // ...
  ],
  // ...
}

Then tell relay-compiler to use the TS plugin and to compile artefacts to the same directory as above:

relay-compiler --src ./ts/ --schema ./data/schema.graphql --language typescript --artifactDirectory ./ts/__relay_artifacts__

Which results in:

tree ts/__relay_artifacts__/
ts/__relay_artifacts__/
├── AddTodoMutation.graphql.ts
├── ChangeTodoStatusMutation.graphql.ts
├── MarkAllTodosMutation.graphql.ts
├── RemoveCompletedTodosMutation.graphql.ts
├── RemoveTodoMutation.graphql.ts
├── RenameTodoMutation.graphql.ts
├── TodoApp_viewer.graphql.ts
├── TodoListFooter_viewer.graphql.ts
├── TodoList_viewer.graphql.ts
├── Todo_todo.graphql.ts
├── Todo_viewer.graphql.ts
└── appQuery.graphql.ts

voxmatt · 2018-01-21T21:35:47Z

Badass

kastermester · 2018-01-22T10:52:56Z

While working on the TypeScript equivalent of the babel plugin I noticed that (at least for ts-loader), the file name that gets passed into the TypeScript transformation context is not actually an absolute path to the file. I need to do some testing to verify whether or not that is the case for babel loader as well. If that is the case then there's some minor changes needed to the babel plugin in order to make it work properly when setting the artifactDirectory config. I will post my findings once I get to it :)

alloy · 2018-01-28T00:25:24Z

@kassens @jstejada Alright, regardless of the webpack thing that @kastermester is still going to look into, this should now be in a good state to review.

alloy · 2018-01-29T10:49:58Z

packages/relay-compiler/RelayCompilerPublic.js

@@ -15,11 +15,12 @@ const GraphQLCompilerContext = require('./graphql-compiler/core/GraphQLCompilerC
 const RelayCodeGenerator = require('./codegen/RelayCodeGenerator');
 const RelayFileWriter = require('./codegen/RelayFileWriter');
 const RelayIRTransforms = require('./core/RelayIRTransforms');


Currently Relay only exports the predefined groups of transforms from https://github.com/facebook/relay/blob/a8af6d7dea1c087bd6c5ba1ca4a2f538d9989cff/packages/relay-compiler/core/RelayIRTransforms.js. However, other language plugins will want to re-use some of these just like the Flow generator does. We are currently doing that in our TS plugin by simply plucking the ones that the Flow generator uses out from the pre-defined groups, but this is fragile and so a more stable solution would be appreciated.

What do you think would be the best way to do this? Export all transformers or is it unlikely that a type generator would need any other builtin transforms than the ones that the Flow generator also uses and so we should just add a predefined group for type generators?

I’m currently leaning towards creating a predefined group for type generators.

alloy · 2018-01-29T11:09:32Z

Rebased on master again.

alloy · 2018-01-29T11:10:28Z

packages/relay-compiler/core/RelayFindGraphQLTags.js

+  if (!condition) {
+    throw new Error(util.format(msg, ...args));
+  }
+}


Any idea why this needed to be defined in FindGraphQLTags.js rather than importing it?

I think it was to allow the compiler to be more restrictive in its dependencies, but I'm not sure

alloy · 2018-02-07T22:23:28Z

@kastermester Do you have an update re the web pack stuff that still needs to be done? Or otherwise some details about what I should look into?

kastermester · 2018-02-08T10:02:14Z

@alloy I haven't had time to look into it yet. But I'd say if you're using the option to put all artifacts in a single directory with the babel plugin and it works - then there shouldn't be any issues :)

alloy · 2018-02-08T10:09:38Z

@kastermester Re-reading your original comment about the web pack issue, it seems like you’re saying that the issue was around determining the path to the destination directory, is that correct? If so, when using the artifactDirectory option there’s no real determination done at all, all artifacts are simply assumed to be siblings of the file being generated. Does that address the issue well enough?

kastermester · 2018-02-08T10:15:49Z

Without using the new artifactDirectory option puts all artifacts in a single directory, where previously it would be put in a directory residing inside the same directory of the file.

This meant requiring artifact files were simply require('./__generated__/SomeFile.graphql.js').

With the artifactDirectory we need some way of finding the single directory where the artifacts are stored. My initial attempt (in the TS plugin) I attempted to use the fileName/filePath property in the TypeScript transformation context and resolve a relative require (ie ../../__generated__/SomeFile.graphql.js). However I discovered that the fileName passed into the transformation context was relative, not absolute - and thus this didn't work.

What I have ended up with (so far) in the TypeScript case is to give the responsibility back to the user. Ie:

In webpack, define an alias __generated__ that always points to the generated directory, do the same (using paths option in tsconfig) for TypeScript, and set the artifactDirectory option to __generated__. Now require's are simply turned into __generated__/SomeFile.graphql which is understood by both TS and Webpack (or similiar system).

What I haven't had time to test yet - is whether the babel loader is passing absolute file names to the file being generated as opposed to the TS loader, or if it exhibits the same issues.

alloy · 2018-02-08T10:25:17Z

Oh I see now what you mean, you’re referring to the runtime requiring of the concrete node, whereas I was referring to the types. We’re using a Babel pipeline in our React Native app and it works well in that case 👍

kastermester · 2018-02-08T10:27:18Z

Ah yes, this relates to the PR relay-tools/relay-compiler-language-typescript#21 for the TS transformer and the babel relay plugin in Relay. Might not have made that clear enough.

leebyron

This looks pretty awesome.
@kassens, care to take a review?

leebyron · 2018-02-08T15:50:49Z

packages/relay-compiler/core/RelayFindGraphQLTags.js

+  if (!condition) {
+    throw new Error(util.format(msg, ...args));
+  }
+}


I think it was to allow the compiler to be more restrictive in its dependencies, but I'm not sure

alloy · 2018-02-10T15:01:49Z

CI is red on master, which is fixed by #2318.

alloy · 2018-02-10T15:15:44Z

Alright, everything has been rebased and tested to work well in:

The TypeScript plugin example: https://github.com/relay-tools/relay-compiler-language-typescript/tree/master/example
A fork of the JS example app: https://github.com/alloy/relay-examples/tree/language-plugin-test/todo

Interesting note about the JS example is that with the addition of the artifactDirectory option in this PR, JS/Flow users now also get the opportunity to have safe fragment references without using Haste: https://github.com/alloy/relay-examples/blob/f8105c243bf476ff0bb0e24de06469ab8e4d62e7/todo/js/__relay_artifacts__/TodoList_viewer.graphql.js#L11-L12

So everybody wins 🎉

facebook-github-bot

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

kastermester · 2018-06-27T14:53:59Z

@jstejada Better have it land properly and well integrated than a half assed merge :) Thank you so much for looking into this. I think everyone here knows how stuff have a habit of getting in the way.

Keep up the good work! :)

alloy · 2018-06-27T15:11:00Z

@jstejada What @kastermester said! Thanks for all the effort thus far ❤️

enisdenjo · 2018-07-25T14:15:38Z

Hey guys, any updates on this merge? We've been eagerly (but yet patiently) waiting for 7 months now...

arichiv · 2018-07-25T14:19:17Z

I left facebook in March 2017, can I re-join for a couple days to merge?

alloy · 2018-07-25T14:29:27Z

FYI I’ll be rebasing this one over the weekend and signal @jstejada that we can continue.

alloy · 2018-07-28T21:51:24Z

Ok

I’ve rebased the PR
Uploaded a build of this PR https://github.com/alloy/relay/releases/tag/v1.6.1-plugin.1
Pushed a new version (0.9.1) of the TS plugin that uses this https://www.npmjs.com/package/relay-compiler-language-typescript

Other than the example app in the plugin repo I have not yet had the time to verify this in our larger apps. If you do in the meantime, please let me know.

facebook-github-bot

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

jstejada · 2018-07-29T20:21:53Z

packages/relay-compiler/language/javascript/formatGeneratedModule.js

@@ -33,12 +36,12 @@ const formatGeneratedModule: FormatModule = ({
 'use strict';

 /*::
-import type { ${documentType} } from '${relayRuntimeModule}';
-${flowText || ''}
+${documentTypeImport}


this would generate incorrect code if documentTypeImport is an empty string. In which cases is documentType null?

As discussed elsewhere, this was basically just following some of the existing code and would indeed generate incorrect code but only if the inputs were already incorrect anyway.

enisdenjo · 2018-08-02T13:32:05Z

Hey @alloy,

I have been using your v1.6.1-plugin.1 in production for 4 days now. Haven't faced any regressions yet!

D.

alloy · 2018-08-02T20:11:25Z

Awww yeah! Thanks for all the assistance shepherding this through @jstejada 🙌 🍾

waterfoul · 2018-08-03T14:47:39Z

Any idea when a release will be cut containing these changes?

soneymathew · 2018-08-05T03:54:56Z

@alloy Will this work for non-ts projects?
Please let me know if this is supported usage
soneymathew/relay-examples@18740d8

kastermester · 2018-08-06T12:15:41Z

@soneymathew The artifactDirectory does not depend on which language plugin you're using. It will simply redirect all the artifact files to a single directory.

Note: If you're using flow this should allow flow to typecheck the fragment types akin to how it works if one were to use the haste module system prior to this merge. In short typechecking is applied to the relay properties based on the fragments being spread (Parent component can't render Child component without having done ...Child_prop in the corresponding fragment).

soneymathew · 2018-08-10T07:39:49Z

@kastermester That matches my understanding but, in this fork(soneymathew/relay-examples@18740d8), I couldn't make it work.

kastermester · 2018-08-10T08:20:47Z

@soneymathew As far as I can see this is due to the fact that there's still not a release of Relay published yet with the language plugin changes merged. 1.6.2 does not contain this PR. I think @alloy has a version of the relay compiler published with this PR applied somewhere that you can try out?

soneymathew · 2018-08-10T08:41:00Z

Thanks @kastermester 🤦‍♂️ I was looking forward for this one way too long and skipped to check that fact...
v1.6.2...master

alloy · 2018-08-10T13:47:19Z

Heyyy @soneymathew, sorry just getting back online after being away for a bit. I don’t have builds of 1.6.2 with this code applied, the last I had was 1.6.1 just before 1.6.2 was released https://github.com/artsy/relay/releases/tag/v1.6.1-plugin.1

merrywhether · 2018-08-24T19:47:57Z

In case no one has seen it, this is included in the relay 1.7.0-rc.1 release! 🎉

https://github.com/facebook/relay/releases/tag/v1.7.0-rc.1

Thanks so much to the relay team and @alloy for all the work on this!

facebook-github-bot added the CLA Signed label Jan 21, 2018

This was referenced Jan 21, 2018

[WIP] Language plugin artsy/relay#5

Closed

Add basic external transformation support to relay-compiler #1710

Closed

alloy mentioned this pull request Jan 22, 2018

[Transform] WIP: Add code to transform away the tagged template literals relay-tools/relay-compiler-language-typescript#21

Open

5 tasks

alloy force-pushed the language-plugin branch 2 times, most recently from 97f9f4a to 97c58bf Compare January 27, 2018 21:43

alloy mentioned this pull request Jan 29, 2018

relay-compiler does not support JSX fragments #2280

Closed

alloy commented Jan 29, 2018

View reviewed changes

alloy force-pushed the language-plugin branch from c19ed6f to 1a085e7 Compare January 29, 2018 11:09

alloy commented Jan 29, 2018

View reviewed changes

alloy force-pushed the language-plugin branch from 1a085e7 to d3780a9 Compare February 3, 2018 22:32

alloy mentioned this pull request Feb 6, 2018

[Relay] Uses latest version and our patches artsy/emission#982

Merged

alloy mentioned this pull request Feb 8, 2018

Nested fragments cause flow errors with relay modern #2316

Closed

alloy changed the title ~~[WIP] Language plugin support.~~ Language plugin support. Feb 8, 2018

leebyron reviewed Feb 8, 2018

View reviewed changes

alloy force-pushed the language-plugin branch from d3780a9 to 7c36930 Compare February 10, 2018 14:40

facebook-github-bot reviewed Feb 12, 2018

View reviewed changes

alloy mentioned this pull request Feb 12, 2018

ReasonML support #2231

Closed

alloy mentioned this pull request Jul 17, 2018

babel-plugin-relay and relay-compiler are not compatible with ES6 modules #2445

Open

[relay-compiler] Add plugin support to parse/generate other languages.

312c1f3

alloy force-pushed the language-plugin branch from 3010855 to 312c1f3 Compare July 28, 2018 18:47

facebook-github-bot reviewed Jul 29, 2018

View reviewed changes

jstejada reviewed Jul 29, 2018

View reviewed changes

kevinhughes27 mentioned this pull request Jul 30, 2018

generate types from graphql queries kevinhughes27/ultimate-tournament#822

Merged

facebook-github-bot closed this in af35b67 Aug 2, 2018

alloy deleted the language-plugin branch August 3, 2018 20:16

sibelius mentioned this pull request Aug 5, 2018

Flow: property $refType is missing in object #2394

Closed

kevinhughes27 mentioned this pull request Aug 6, 2018

update to the new relay release. kevinhughes27/ultimate-tournament#827

Closed

kevinhughes27 mentioned this pull request Aug 24, 2018

update relay kevinhughes27/ultimate-tournament#837

Merged

koddsson mentioned this pull request Dec 6, 2018

Support TypeScript relay-tools/relay-compiler-webpack-plugin#33

Closed

jnak mentioned this pull request Dec 19, 2018

Add support for artifactDirectory option relay-tools/relay-compiler-webpack-plugin#38

Merged

Language plugin support. #2293

Language plugin support. #2293

Conversation

alloy commented Jan 21, 2018 • edited Loading

Test plan

voxmatt commented Jan 21, 2018

kastermester commented Jan 22, 2018

alloy commented Jan 28, 2018

alloy Jan 29, 2018

Choose a reason for hiding this comment

alloy Jan 29, 2018

Choose a reason for hiding this comment

alloy commented Jan 29, 2018

alloy Jan 29, 2018

Choose a reason for hiding this comment

leebyron Feb 8, 2018

Choose a reason for hiding this comment

alloy commented Feb 7, 2018

kastermester commented Feb 8, 2018

alloy commented Feb 8, 2018

kastermester commented Feb 8, 2018

alloy commented Feb 8, 2018 • edited Loading

kastermester commented Feb 8, 2018

leebyron left a comment

Choose a reason for hiding this comment

leebyron Feb 8, 2018

Choose a reason for hiding this comment

alloy commented Feb 10, 2018

alloy commented Feb 10, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

kastermester commented Jun 27, 2018

alloy commented Jun 27, 2018

enisdenjo commented Jul 25, 2018

arichiv commented Jul 25, 2018

alloy commented Jul 25, 2018

alloy commented Jul 28, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

jstejada Jul 29, 2018 • edited Loading

Choose a reason for hiding this comment

alloy Jul 30, 2018

Choose a reason for hiding this comment

enisdenjo commented Aug 2, 2018

alloy commented Aug 2, 2018

waterfoul commented Aug 3, 2018

soneymathew commented Aug 5, 2018

kastermester commented Aug 6, 2018

soneymathew commented Aug 10, 2018

kastermester commented Aug 10, 2018

soneymathew commented Aug 10, 2018

alloy commented Aug 10, 2018

merrywhether commented Aug 24, 2018

alloy commented Jan 21, 2018 •

edited

Loading

alloy commented Feb 8, 2018 •

edited

Loading

jstejada Jul 29, 2018 •

edited

Loading