Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add basic external transformation support to relay-compiler #1710

Closed
wants to merge 1 commit into from
Closed

Add basic external transformation support to relay-compiler #1710

wants to merge 1 commit into from

Conversation

s-panferov
Copy link

@s-panferov s-panferov commented Apr 30, 2017

Hello, thanks for the great library. We are using TypeScript for our codebase and it will be very useful to enable custom pre-transformations for a source code before parsing, so we can compile our typescript modules without complex configuration. The PR adds such support.

It is a very basic implementation for discussion purposes.

Proposed usage:

yarn add  relay-compiler-typescript
relay-compiler --transform relay-compiler-typescript --extensions ts tsx js jsx

You can find example transformer implementation here: https://github.com/s-panferov/relay-compiler-typescript/tree/master

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@leebyron
Copy link
Contributor

Very interesting! Could you ensure lint passes?

@s-panferov
Copy link
Author

s-panferov commented May 13, 2017

@leebyron it fails on a global variable which linter doesn't know (__non_webpack_require__). I'll try to figure out how to add a global variable to the linter

@miracle2k
Copy link

While I've had very good success so far just running relay-compiler over my .ts and .tsx files, one thing relay-compiler cannot parse is a foo as any cast in a .tsx file.

@kassens
Copy link
Member

kassens commented May 26, 2017

I'm starting work on simplifying the graphql extraction to no longer fully parse which should also make it work for ts! \o/

@voxmatt
Copy link
Contributor

voxmatt commented Jun 6, 2017

Any updates here? This is super-interesting.

let moduleImpl: TransformFactory
try {
// $FlowFixMe flow doesn't know about __non_webpack_require__
moduleImpl = (__non_webpack_require__(moduleName): TransformFactory)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @wincent - could we use this to require the persist module?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably. I need to re-test my PR. If it doesn't work can try this "one weird trick".

@kassens
Copy link
Member

kassens commented Jun 6, 2017

Sorry for the delay here. My plan to pull out the GraphQL tags using a simplified state machine parser didn't work out because I wanted to avoid false positives such as a graphql`` tag appearing inside a string or other edge cases.

I'll send a commit soon that enables parsing of pure .graphql files using a second parser config instead of RelayFileIRParser. This should show pretty clearly how you could add a TypescriptFileIRParser.

@bchenSyd
Copy link

bchenSyd commented Jun 6, 2017

mocha has an option called --compilers where you specify the file extension and a js bootstrap file (mostly babel-register and node-ts/register) which hooks up to node's module resolution method.

after that, node will consult the right transpiler when it resolve a module. for all registered module files like .js/.jsx/.ts/.tsx files node will call the corresponding transform method before putting into its module system. so from mocha's point of view, everything is plain es5.

i think its a good solution. dont call their transform function explicitly, but let node calls their module hook ups and only take es5 as input. I think jest and relay-compiler can take the same approach

@voxmatt
Copy link
Contributor

voxmatt commented Jun 6, 2017

@kassens (or anyone) have you figured out a work-around in the meantime. Without the ability to run relay-compiler against our Typescript codebase, it's not really possible to run in development mode (we use webpack dev server).

@jdolle
Copy link

jdolle commented Jun 6, 2017

@voxmatt I am currently working on this very problem. My workaround was to move the graphql call to a .js file and then import that in my component.ts. You have to set allowJs: true in the tsconfig but otherwise seems like a good temporary solution. Let me know if that works for you.

@alloy alloy mentioned this pull request Jun 13, 2017
20 tasks
@alloy
Copy link
Contributor

alloy commented Jun 13, 2017

Rebased this branch and fixed the linter issues https://github.com/s-panferov/relay/pull/1

@lukewlms
Copy link

lukewlms commented Jun 19, 2017

@jdolle @voxmatt Could you explain to me the complication you're trying to fix? We have this simple build config set up under npm build and it works well with Relay, but there may be an optimization we're lacking.

  1. tsc (src -> dist)
  2. relay-compiler on JS files in dist directory
  3. babel on JS files in dist directory

Our project is still fairly simple, are there aspects of more complicated projects you're seeing where this setup breaks?

In our tsconfig.json we use these to make sure graphql tags are properly picked up by Babel:

    "target": "esnext", // babel handles downtranspiling
    // module: es2015 is required for TypeScript to properly output graphql
    // tags
    "module": "es2015", 

@voxmatt
Copy link
Contributor

voxmatt commented Jun 19, 2017

When using webpack-dev-server, there is no dist folder to run the relay-compiler on. We need either (a) some way to run the compiler against ts files without failing (this is preferable and seems pretty close to doable since it's just getting hung up on superfluous syntax) or (b) run the compiler in a step that webpack runs.

Previously, with relay-classic, this worked because relay relied on the babel plugin alone and babel is a loader in our webpack config; so, the babel step was handled in the dev-server setup.

@lukewlms
Copy link

@voxmatt Very much appreciate the explanation - clipped for when we're building out the web version of our app.

alloy added a commit to artsy/emission that referenced this pull request Jul 25, 2017
alloy added a commit to artsy/emission that referenced this pull request Aug 14, 2017
alloy added a commit to artsy/emission that referenced this pull request Aug 25, 2017
alloy added a commit to artsy/emission that referenced this pull request Sep 4, 2017
alloy added a commit to artsy/emission that referenced this pull request Sep 4, 2017
@leebyron leebyron assigned wincent and unassigned kassens Sep 7, 2017
@leebyron
Copy link
Contributor

leebyron commented Sep 7, 2017

Hey @wincent, could you please look at this issue? We should make a decision as to whether this fits into the relay-compiler or not

@leebyron
Copy link
Contributor

leebyron commented Sep 7, 2017

Also - this PR no longer merges cleanly. @s-panferov would you mind rebasing?

@wincent
Copy link
Contributor

wincent commented Sep 8, 2017

Hey @wincent, could you please look at this issue? We should make a decision as to whether this fits into the relay-compiler or not

I think it makes sense for something like this to be in "relay-compiler". Even if we don't go all the way to having a general pluggable transform mechanism like this, we know the concrete TypeScript use-case would be valuable to solve.

For me the main question is which of these is most desirable:

  • Generic fast parser as proposed by @kassens (which he ended up punting on due to concerns about false positives).
  • Pluggable transform as shown in this PR.
  • Closely related to this idea of augmenting the parser, making it simple to replace it by creating and using a simple TypeScriptParser (which could either be a true parser like RelayJSModuleParser which ultimately uses Babylon to parse JS, or a crude text-based matcher like this one). For reference, the simplest alternative parser is the DotGraphQLParser.

My gut sense that the second path (the one implemented in this PR) feels like a good approach. We don't yet have a good story for how you can swap pieces in and out of the compiler (short of making your own module which involved a bit of copy-pasta), and adding a --transform switch doesn't seem like too much API creep.

@alloy
Copy link
Contributor

alloy commented Sep 8, 2017

In our case I decided to go with a very simple stopgap solution and start experimenting with Babel 7, which has a TS parser, asap.

@robrichard
Copy link
Contributor

Should this feature be called something other than transform to avoid confusion with compiler transforms?

@miracle2k
Copy link

I ported this patch to relay 1.3.0, get it here: miracle2k@eda4f6b

baluubas pushed a commit to baluubas/relay that referenced this pull request Oct 12, 2017
@jimkyndemeyer
Copy link

@alloy Do you have an update on using Babel 7? We use TypeScript with Relay Classic and this issue is preventing us from upgrading to Modern.

@alloy
Copy link
Contributor

alloy commented Oct 18, 2017

@jimkyndemeyer Oh yes! I had ported our React Native app to use Haul, which is webpack based, and was able to use Babel 7 without tsc with great success artsy/emission#780 🎉

The main issue I encountered was that the TS and Flow plugins for Babel can’t be active at the same time, so I had to strip all node_modules of flow typings that were present in libraries after installing dependencies, but iirc Relay doesn’t ship with Flow typings left in (only React and React Native do), so that shouldn’t be an issue.

The reason I haven’t brought it to completion yet is that we decided to switch back to the default RN packager (Metro) and alas I haven’t yet been able to get Babel 7 to work with that setup, but I didn’t spend much time on that yet either. If you use webpack, this shouldn’t be an issue for you and you can largely follow what I did in that linked PR.

Addendum: the Babel 7 TS plugin does not support namespaces and const enums, so if you require those you’re out of luck.

@mattias800
Copy link

Any news on this? Would be great if we got proper TS support in the Relay compiler.

I've written a simple Typescript transformer which replaces the babel relay plugin. You can use it if you want to use the TS compiler instead of Babel 7.
https://github.com/mattias800/relay-modern-typescript-transformer

@darting
Copy link

darting commented Jan 2, 2018

Hi all .. any news on this? many thanks

@alloy
Copy link
Contributor

alloy commented Jan 2, 2018

As part of work I’m finishing up over the week to add TS type info to artefacts, I’m also working on a very simple plugin API, but in fact my work will also already support TS parsing by using Babel 7.

@alloy alloy mentioned this pull request Jan 21, 2018
4 tasks
@alloy
Copy link
Contributor

alloy commented Jan 21, 2018

For people subscribed to this PR, I just pushed a new PR that adds plugin support and a link to a TS plugin. #2293

@alloy
Copy link
Contributor

alloy commented Jan 28, 2018

@jstejada I would say that by now this can be closed in favour of #2293.

@alloy
Copy link
Contributor

alloy commented Jan 28, 2018

Wow, I don’t know how I could have unassigned @wincent 👻

screen shot 2018-01-28 at 20 58 16

🤷‍♂️

@jstejada jstejada closed this Jan 28, 2018
facebook-github-bot pushed a commit that referenced this pull request Aug 2, 2018
Summary:
Closes #2073.
Closes #1710.

In preparation for #2073 and [as requested by kassens](http://twitter.com/kassens/status/946811879204245504), 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](https://github.com/alloy/relay/blob/d17bfdb18fe45bae531b13487ba5b9efa1c96171/packages/relay-compiler/language/RelayLanguagePluginInterface.js) 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](https://github.com/facebook/relay/pull/2293/files#diff-2c18fc91f11394e307550103afff9722R56) (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/kastermester/relay-compiler-language-typescript. (You can find example artifacts [here](https://github.com/kastermester/relay-compiler-language-typescript/tree/master/example/ts/__generated__).)

----

Things that are left to do:
- [x] Document all of the types in the plugin interface
- [x] Remove version changes from `package.json` files
- [x] Rebase on latest upstream master
- [x] Verify that before and after JS/Flow artefacts are identical

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

Pull Request resolved: #2293

Reviewed By: kassens

Differential Revision: D6962199

Pulled By: jstejada

fbshipit-source-id: b927a0c02eac914bce6d46ce25d90433fc0bb98f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.