[WIP] Language plugin #5

alloy · 2018-01-16T13:14:46Z

@kastermester Here’s my first pass at the plugin architecture, redone from scratch to leave out any of the TS work I had also done previously.

alloy · 2018-01-16T13:16:18Z

packages/relay-compiler/language/RelayLanguagePluginInterface.js

+  typeGenerator: TypeGenerator,
+  formatModule: FormatModule,
+  findGraphQLTags: GraphQLTagFinder,
+};


This is the interface that the plugin needs to implement.

alloy · 2018-01-16T13:21:54Z

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

@@ -451,5 +450,5 @@ const FLOW_TRANSFORMS: Array<IRTransform> = [

 module.exports = {
  generate: Profiler.instrument(generate, 'RelayFlowGenerator.generate'),
-  flowTransforms: FLOW_TRANSFORMS,
+  transforms: FLOW_TRANSFORMS,


We probably need to use the exact same transforms for our TS plugin. Seeing as the compiler pubic API already exports all of these in predefined sets, I’m inclined to just export all of them individually as well.

Yeah I believe I used the same transforms as flow in the example I had running. I think passing a "recommended" set of transforms, possibly along with other transforms that could be deemed useful might be a good idea? Either way - I believe all needed for the TS plugin would be the same transforms as the flow type generator uses.

Well… that’s actually a lot more involved, so I’d probably want to discuss that with the Relay team. For now, let’s just pick these few from the currently exported list. i.e. this should work:

import { IRTransforms } from "relay-compiler" const transforms = [ IRTransforms.commonTransforms[2], // RelayRelayDirectiveTransform.transform IRTransforms.commonTransforms[3], // RelayMaskTransform.transform IRTransforms.printTransforms[0], // FlattenTransform.transformWithOptions({}) ]

I think passing a "recommended" set of transforms, possibly along with other transforms that could be deemed useful might be a good idea?

Exactly, something along those lines. We’ll suggest something to the team and see what they think is best 👍

I guess something like that could be done :) Although a named export seems a bit more robust. I will focus on getting something to work to begin with - that probably exposes a few areas where we could make few changes to make things easier and more discoverable :)

Oh totally! I just meant for the interim while we work on the plugin side of things. When we submit this plugin PR upstream I would definitely want to use a more robust solution.

kastermester · 2018-01-16T13:28:49Z

Having just glanced at it, it looks very promising! I will begin work on a TS plugin as soon as I get some time. But it looks like much of the code I shared with you can be used for this.

Some quick comments/questions:

Would it be possible to make the memoizedFind part of the module parser part of the relay-compiler implementation, such that I wouldn't need to implement this myself? (to get the same performance characteristics as the JS module parser) ? I'm not sure if you could simply wrap the plugins findGraphQLTags function?
It could be possible to avoid double parsing the graphql if the findGraphQLTags function of the plugin could receive the contents of the graphql package as required by the relay-compiler, and then changing the return type of the findGraphQLTags to be Array<OperationDefinitionNode | FragmentDefinitionNode>. I'm not sure whether this is worth it - but it could be worth considering.

So far I like the looks of this very much though! It should be relatively easy to build something on top of this. I will try to get a working example running ASAP, so we can make sure we get all the relevant/needed hooks implemented.

alloy · 2018-01-16T13:40:17Z

Would it be possible to make the memoizedFind part of the module parser part of the relay-compiler implementation, such that I wouldn't need to implement this myself? (to get the same performance characteristics as the JS module parser) ? I'm not sure if you could simply wrap the plugins findGraphQLTags function?

Yup, makes sense 👍

It could be possible to avoid double parsing the graphql if the findGraphQLTags function of the plugin could receive the contents of the graphql package as required by the relay-compiler, and then changing the return type of the findGraphQLTags to be Array<OperationDefinitionNode | FragmentDefinitionNode>. I'm not sure whether this is worth it - but it could be worth considering.

Sounds like we should indeed do this for the final version. I’m wondering if it makes sense to do this iteratively such that we can work on the TS plugin first and then get back to this PR to make the final tweaks needed to polish it?

kastermester · 2018-01-16T13:41:51Z

Sounds like we should indeed do this for the final version. I’m wondering if it makes sense to do this iteratively such that we can work on the TS plugin first and then get back to this PR to make the final tweaks needed to polish it?

Sounds like a plan. It wasn't meant like it had to be done right now, it was just a quick observation.

kastermester · 2018-01-17T09:15:53Z

I hope I can get to spend some time on this during this week, but I might get a bit pressured on time, we'll see. I'll let you know the second I have something worth looking at.

alloy · 2018-01-17T10:55:26Z

@kastermester I may have some time to get started on making your repo conform to the plugin interface, but I don’t want to step on your toes, so let me know if/when you start on something.

kastermester · 2018-01-17T11:40:46Z

@alloy Earliest I should get the chance to look at it will be this weekend (should have some time saturday I would think).

I will merge your pull request and if you have the time to take a stab at it then that would be great :)

I think as soon as we have something running in the repo that sort of works - we should look into porting as many tests as possible from the relay-compiler repo - I'm just not sure where to start with that.

alloy · 2018-01-17T11:44:03Z

@kastermester Alright, I made memoizedFind part of the source module parser instead. With regards to the double GraphQL parsing, are you referring to FindGraphQLTags doing its own parsing of the fragment to validate the name?

Come to think of it, the name validation also shouldn’t be left up to the plugin, the only thing the plugin should do is extract the GraphQL fragment.

kastermester · 2018-01-17T11:51:05Z

Sounds good!

Yes I was thinking that the plugin needs to do its own validation for now. If we wish to rework this, perhaps the return type from the plugin in find graphql tags should be something like:

type GraphQLTagLocation = GraphQLTagFreeForm | GraphQLTagComponent | GraphQLTagComponentProp;
interface GraphQLTagFreeForm {
  location: 'freeform';
}
interface GraphQLTagComponent {
  location: 'component';
  componentName: string;
}
interface GraphQLTagComponentProp {
  location: 'componentProp';
  componentName: string;
  propName: string;
}
interface GraphQLTaggedText {
  location: GraphQLTagLocation;
  graphQLText: string;
}

declare function findGraphQLTags(...): Array<GraphQLTaggedText>;

This way all the validation could be left inside the relay-compiler package and the various plugins can just deal with figuring out where the tags originated from.

Note: I'm not saying the names for the interfaces and types are the right choices for them - I figure there's some better options.

alloy · 2018-01-19T10:34:15Z

@kastermester I moved name validation out of the plugin’s responsibility. I didn’t yet optimise the GraphQL parsing, but that can now be done at a later time independently, the plugin now only needs to find the tag and return just that.

alloy · 2018-01-19T11:10:59Z

Alright, just did some testing comparing artefacts produced by the upstream 1.5.0-rc.1 release and this plugin code; everything seems good, there’s no differences in generated artefacts.

alloy · 2018-01-19T11:11:54Z

Going to take a look at updating your code to conform to the plugin interface.

kastermester · 2018-01-19T11:12:44Z

Sounds great! I expect to have some time tomorrow to work on this as well :)

E.g. the TypeScript compiler API requires it.

When this is done, the flow generator can import fragment reference types - to get same type checking as when using haste. Also added an option to the babel plugin, enabling it to require the correct file path.

Add CLI option to set outputDir of RelayFileWriter

CLI interface polish

alloy · 2018-01-21T00:19:22Z

Alright, I’ve submitted this code upstream facebook#2293

Will continue the WIP there instead.

alloy added 2 commits January 15, 2018 15:08

[language-plugin] Move JS/Flow code into language dir.

1f89fd4

[language-plugin] Add interface and JS/Flow implementation.

37c7c33

alloy commented Jan 16, 2018

View reviewed changes

[language-plugin] Don’t leave GraphQL tag caching up to plugins.

06bf77f

alloy mentioned this pull request Jan 17, 2018

Test coverage relay-tools/relay-compiler-language-typescript#5

Closed

alloy mentioned this pull request Jan 17, 2018

relay-compiler plugin support relay-tools/relay-compiler-language-typescript#6

Closed

[language-plugin] Don’t leave fragment name validation up to plugins.

c4b2d5c

alloy force-pushed the language-plugin branch from bd4ea21 to c4b2d5c Compare January 19, 2018 10:22

[language-plugin] Move ‘format module’ code to plugin.

72e0f9f

[package] Update versions to 1.4.1-plugin.2

d70de4a

alloy mentioned this pull request Jan 19, 2018

Import plugin interface relay-tools/relay-compiler-language-typescript#9

Merged

alloy added 6 commits January 19, 2018 17:56

[language-plugin] Move Flow generator tests to right location.

5f809fb

[language-plugin] Move type generator options to plugin interface.

194b34d

[language-plugin] Pass file path to GraphQL tag finder.

8c9fd6a

E.g. the TypeScript compiler API requires it.

[language-plugin] Load ES6 modules in uniform way.

49c5392

[language-plugin] Prefix plugin packages.

43aeafb

[package] Bump to 1.4.1-plugin.3

a6ec97e

kastermester and others added 13 commits January 20, 2018 18:43

Add CLI option to set outputDir of RelayFileWriter

b064b0d

When this is done, the flow generator can import fragment reference types - to get same type checking as when using haste. Also added an option to the babel plugin, enabling it to require the correct file path.

Prettier format.

f019a1c

Merge pull request #6 from kastermester/language-plugin-with-output-dir

ac5cf98

Add CLI option to set outputDir of RelayFileWriter

[bin] Fix type error due to prettier formatting.

442873a

[Compiler] Improve bin interface.

631374c

[Compiler] Default artifactDirectory to .babelrc setting.

85f2381

[Compiler] Revert defaulting artifactDirectory to .babelrc

de2e910

Merge pull request #7 from alloy/language-plugin-output-dir

be12958

CLI interface polish

[package] Bump to 1.4.1-plugin.4

5b3a1cc

[language-plugin] The plugin has no need for the artifact dir.

ab0d4a1

[package] Bump to 1.4.1-plugin.5

505aad3

[BabelPlugin] Ensure import path is always relative.

11c29b2

[package] Bump to 1.4.1-plugin.6

d17bfdb

alloy closed this Jan 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Language plugin #5

[WIP] Language plugin #5

alloy commented Jan 16, 2018 •

edited

Loading

alloy Jan 16, 2018

alloy Jan 16, 2018

kastermester Jan 16, 2018

alloy Jan 16, 2018

alloy Jan 16, 2018

kastermester Jan 16, 2018

alloy Jan 16, 2018

kastermester commented Jan 16, 2018 •

edited by alloy

Loading

alloy commented Jan 16, 2018

kastermester commented Jan 16, 2018 •

edited

Loading

kastermester commented Jan 17, 2018

alloy commented Jan 17, 2018

kastermester commented Jan 17, 2018

alloy commented Jan 17, 2018

kastermester commented Jan 17, 2018

alloy commented Jan 19, 2018

alloy commented Jan 19, 2018

alloy commented Jan 19, 2018

kastermester commented Jan 19, 2018

alloy commented Jan 21, 2018

[WIP] Language plugin #5

[WIP] Language plugin #5

Conversation

alloy commented Jan 16, 2018 • edited Loading

alloy Jan 16, 2018

Choose a reason for hiding this comment

alloy Jan 16, 2018

Choose a reason for hiding this comment

kastermester Jan 16, 2018

Choose a reason for hiding this comment

alloy Jan 16, 2018

Choose a reason for hiding this comment

alloy Jan 16, 2018

Choose a reason for hiding this comment

kastermester Jan 16, 2018

Choose a reason for hiding this comment

alloy Jan 16, 2018

Choose a reason for hiding this comment

kastermester commented Jan 16, 2018 • edited by alloy Loading

alloy commented Jan 16, 2018

kastermester commented Jan 16, 2018 • edited Loading

kastermester commented Jan 17, 2018

alloy commented Jan 17, 2018

kastermester commented Jan 17, 2018

alloy commented Jan 17, 2018

kastermester commented Jan 17, 2018

alloy commented Jan 19, 2018

alloy commented Jan 19, 2018

alloy commented Jan 19, 2018

kastermester commented Jan 19, 2018

alloy commented Jan 21, 2018

alloy commented Jan 16, 2018 •

edited

Loading

kastermester commented Jan 16, 2018 •

edited by alloy

Loading

kastermester commented Jan 16, 2018 •

edited

Loading