[Transform] WIP: Add code to transform away the tagged template literals #21
Conversation
|
Awesome, looks promising 👏 |
|
@alloy I've ported tons more of the code needed to support classic/compat mode. I've noticed that the classic code is doing some static analysis (which seems like it is not strictly necessary to do so) on the code in classic/compat mode, seemingly to support referencing fragments in both classic and compat mode. I think we will need to make the example app in both modern only, classic only and compat mode to test this transform properly once done. Perhaps it would be a good idea to have it running under the babel code first as well, so we have a working starting point to go from. But all in all, I would guess that I can have this working within 1-2 more days of work. |
|
@alloy Here's code that can do something for both classic, compat and modern code. |
|
I created some very rudimentary and quite a poor man's solution to figure out if some name is defined in a scope in the code (which seems to be needed for compat code). I especially need some tests for this part of the code, preferably both positive and negative tests. Knowing programming languages to some decent degree, I can already think of some cases that are not covered, I would however think that they shouldn't really be relevant in real world code. |
|
@alloy I have imported the test fixtures from babel-plugin-relay (modern, classic and compat) and have tried to go through the snapshot results by comparing them to the upstream result. There's a lot of snapshot stuff to go through and I must admit I haven't methodically looked through them all. I did however find a couple of bugs related to error handling and expression interpolation that is also fixed. I'd say this is far enough along that the next step is probably to try and get modern, compat and classic apps running on it. |
|
Impressive effort! Yeah I agree that the best way forward would be to start using it in actual apps. Perhaps the first step being to move this package to a new repo and porting the example app from this repo 3 times, once for classic, once for compat, and once for modern? |
|
We can move it to a new repo if you think that would be better, I was actually beginning to wonder if the code should just reside in the same npm package as the plugin. It has more or less the same dependencies, and the way one uses the transformers in typescript it should be fairly easy to make it work that way. But I agree we need 3x test apps (starting from the example) using all 3 modes. I’ll see if I get the time later today to set this up. |
A very simple example with just using modern transform implemented.
Moving closer and closer to classic/compat mode being supported. Also fixed some minor stylistic issues in the generated code.
something for both classic/modern/compat
kastermester
force-pushed
the
ts-transform
branch
from
72bb0e0
to
eba2292
Compare
|
I have rebased my changes + made a few more corrections. Initial tests on the example code with this in both modern and compat mode works the same with this and the babel transform. We just need to figure out where and how to publish this package and those examples. I also found that when using webpack - the filename provided here is not an absolute path as I thought, this means that I cannot calculate the relative path to the artifact files - this is also likely an issue in the babel plugin that we need to fix. If we change the meaning of the artifactDirectory config to be a path that resolves to the artifact directory then everything works. ie. it can be either the absolute path to the directory, or some prefix configured as an alias in webpack. |
|
I was thinking of putting it in a separate repo more because of the name for the repo and package being different, but I guess the repo could get renamed again. Maybe it does make working with the two packages across multiple examples apps simpler (in combination with e.g. Lerna). |
Oh, this doesn’t sound good. I’m not sure I entirely follow, but it sounds like you’re saying that facebook/relay#2293 as-is wouldn’t work when people use web pack? If so, can you put some details on that PR so we can track it there? |
The way I see it we have 3 options:
I think that 1. or 3. seems like the right choices. 1. would keep the "mental" footprint down for people having to use this (if they don't want to use babel). If that is not a concern then I think we should go with option 3. There shouldn't be that much need to "coordinate" between the two packages, there's not that many changes that would make the transform incompatible with the plugin. |
Agreed. I need to do some testing to verify whether or not this is an issue when using babel also. It might work differently there. Posted a comment: facebook/relay#2293 (comment) |
We can’t really do 1 because the relay-compiler plugin architecture requires the plugin to be loaded from the root module, so that would get awkward when also trying to contain the transformer in that package. Why not option 2? |
Thanks! |
We could just let the transformer be
It's certainly an option, I just can't really spot any benefits from this option? But if they are there then certainly :) |
|
I’d definitely prefer to have 2 packages, at some point there’s bound to be e.g. a dependency that only applies to one of the two and then people have to pay the price for that even when they don’t use it. (I have strong opinions about dependencies 😉) I guess option 2 made sense to me from the perspective of being able to have 1 setup for all the permutations of plugin/transformer and examples, but that’s not speaking from any experience with such a setup 🤷♂️ |
That makes total sense, let's settle on 2 packages then! 👍
I don't really have any experience with that kind of thing either, perhaps that should be our first step with this? To try and make a setup and see how it feels and how it is working with it? |
|
Sounds good 👍 |
|
I just discovered that this exists, what a clever idea 👍 |
|
I still need to put some more time into verifying that this works as intended. If anyone is using this (somehow, I know I haven't actually published a package etc.) then please let me know how it is working for you. Sadly I don't currently use Relay modern anywhere so I don't have a proper place to test this (we're still on relay classic at work...). |
|
what is missing to this PR? should we use babel 7 instead of typescript to transform? |
|
I just updated the code in the PR to work with newer TS and GraphQL versions. @sibelius If you're up for it I would very much like some feedback on the transformer, so pulling down the code and building it and then running using the transformer would be great. I don't think we're ever gonna discourage people from using Babel to transform the Relay code, this transformer is here for those who wishes to use it. What's missing is:
I know not many people use the compat layer of relay - which is one of the reasons why I'm hoping to get to test this on our product at work. We have a tentative schedule to switch from Relay 0.10 to Relay proper sometime during this year - where we will need to run on Compat for a while. I aim to have us running using this transformer to get it properly tested. In the meantime, again, if you're up for it please try and use it (easiest way is:
cd transformer
npm install && ./node_modules/.bin/tsc
npm packNow you get a tgz ( With regards to how to configure your app - this is also something this PR needs more info on. If you know how transformers this should be familiar to you. The options are the same as they are to the babel transformer, given that the code is simply ported from there. I will try to provide more help once I get more time to look at this, for now I simply took a bit of time to put the code in a workable state. Sorry for the lack of activity on my part with this, but I simply haven't had the time. |
|
Sadly I still have not had too much time to work more on this. Luckily it is not like this code is strictly needed to use Relay with Typescript. I do intend on completing this PR when I feel more confident that the code works. We are going to use it for our product where I work - but it is a slow process to get us using the compat layer (which is the part where the most testing is needed though). In the meantime if anyone is adventurous enough to try and use this - any feedback is greatly appreciated. |
|
What's holding this up? Anything I can assist with @kastermester @alloy @sibelius ?? I really like the sound of this... 🎉 |
|
We need some more people to try this out - mainly on the newest version of relay. We’re using this where I work in our product, but it will be a while before we can upgrade relay past the 1.x branch. If you would be able to help with the testing that it would be great. Once that is done there’s also some managing of the package we need to work on, but we can talk about that once we’re closer. Do note all the relay classic transform stuff should not really matter once using relay 2.x or newer, which should make this package much much simpler. |
A very simple example with just using modern transform implemented.
Here's the initial work - and where I'll track the progress:
I need to copy over some tests from the babel plugin, also I'd like to get a more automated way of having snapshots for various configurations using the same files.