-
Notifications
You must be signed in to change notification settings - Fork 70
Conversation
Looks great!
Like I said I expect to be able to work on this tomorrow - let me know if there’s anything in particular you’d like me to work on. There’s also always the option of implementing the actual TS transformer that we will need, such that we can actually run a Relay application. |
Also just realized we might want to add TSLint to the project as well. |
Yeah that’s always a tough question, which in this case is one to ask the Relay team.
Oh I definitely think it’s a good idea to have the types that are needed by the plugin in this repo, just maybe not all the typings for e.g.
Ace 👍
Sounds like a good idea. Will you add that? |
} | ||
|
||
~~~~~~~~~~ OUTPUT ~~~~~~~~~~ | ||
TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is generating an error when ran currently.
} | ||
|
||
~~~~~~~~~~ OUTPUT ~~~~~~~~~~ | ||
TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is generating an error when ran currently.
} | ||
|
||
~~~~~~~~~~ OUTPUT ~~~~~~~~~~ | ||
TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is generating an error when ran currently.
There seems to be a bug somewhere (either jest itself or the custom snapshot code in relay-test-utils) which leads to the snapshots always being updated and thus the tests seemingly being green when in fact the output does not match. |
src/TypeScriptTypeTransformers.ts
Outdated
enumsHasteModule: string | null | undefined; | ||
existingFragmentNames: Set<string>; | ||
inputFieldWhiteList: ReadonlyArray<string>; | ||
relayRuntimeModule: string; | ||
// TODO: Marking these as optional until we determine how we can best make imports work. | ||
getGeneratedDirectory?: (name: string) => CodegenDirectory; | ||
destinationDirectory?: CodegenDirectory; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kastermester The last few tests were failing because I wasn’t passing in these that you added. Seeing as relay-compiler doesn’t pass these currently (we can add them of course) and we’re not entirely sure yet how to deal with imported files that have changed location, I’ve disabled the code that uses them for now by making these optional and adding guards for them.
21148a4
to
e29ed58
Compare
I think this is ready to be merged and in a state where you can do work on it tomorrow (today).
Things to do include:
|
Oh, wait, no; this is the dreaded situation where you have multiple versions of the same package installed. After fixing that the plugin works:
✨ |
I’ going to do a bit more work on porting the example app over, but if you get to reviewing and merging this PR before I finish that then please do so, the example app work can go into a new PR. |
Found one little bug in the current |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems good! I left a few comments. I had some issues with installing the packages that were npm pack'd, the hashes weren't the right ones, but I'm not sure if it was because you had installed from older versions of the branches, or if the hash just differs between our computers for some other reasons. Either way I just moved the lock files when I installed it and it all seemed to work just fine.
createContainerName(callExpr), | ||
taggedTemplate.tag.getText() | ||
); | ||
addGraphQLTag({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does the relay-compiler package know which container name this came from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t quite follow, can you elaborate?
taggedTemplate.tag.getText() | ||
); | ||
addGraphQLTag({ | ||
keyName: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this should also carry a container name with it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, I thought I had left a comment for this, your original code did not perform any name validation in this code branch and thus there was no keyName
variable yet either. Seeing as it wasn’t immediately obvious to me what type of tag this is I wanted to leave it for you to amend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wait, hah no that’s below.
I assume there’s no keyName
here because this is the code branch for when you specify the fragment spec using only a tagged template, i.e. without a map.
Probably would be helpful to add some comments to these various branches and have tests for each of them.
export type State = { | ||
usedEnums: { [name: string]: GraphQLEnumType }; | ||
usedFragments: Set<string>; | ||
// TODO: Marking these as optional and moved from ‘options’ to ‘state’ until we determine how we can best make imports |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good. There's quite a few options here to be honest, and given that we'd need to recompile a file when something it refers to is moved, perhaps my idea of just resolving the relative paths aren't even that good. But there are some other options here as well. We should probably talk to the relay team about this, such that we can also get it fixed for the flow version when not using haste (which i figure most people aren't).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Let’s ask for feedback once we have everything except for this part working.
Im curious to know, what are your plans regarding the runtime transformation of the graphql tags? AFAIK not even the official babel one would work, as that also expects commonjs exports. Should i begin working on a typescript plugin that can do what we need? This would also eliminate the need for Babel altogether (which I quite like to be honest). |
} | ||
|
||
~~~~~~~~~~ OUTPUT ~~~~~~~~~~ | ||
export type PersonalityTraits = CHEERFUL | DERISIVE | HELPFUL | SNARKY | %future added value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the bug you're refering to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not, but it’s a good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the bug I was referring to 360a291
I’m not entirely sure if I understood you correct, but I think you’re referring to the issue that I fixed here? FWIW the example app works at runtime.
While I think that’s a great idea, I think we can finish this stuff first, seeing as the example app works? (I’m biased, though, as our apps require the babel pipeline for other stuff.) |
example/tsconfig.json
Outdated
"module": "commonjs", /* Specify module code generation: 'none', 'commonjs', 'amd', 'system', 'umd', 'es2015', or 'ESNext'. */ | ||
// "lib": [], /* Specify library files to be included in the compilation: */ | ||
"target": "es2015", /* Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017', or 'ESNEXT'. */ | ||
"module": "es2015", /* Specify module code generation: 'none', 'commonjs', 'amd', 'system', 'umd', 'es2015', or 'ESNext'. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s very important to configure TS to build ES6 modules, otherwise the use of the graphql
function would be transpiled like so:
var relay_runtime_1 = require('relay-runtime');
relay_runtime_1.graphql(…);
Which makes it impossible for babel-plugin-relay
to pick these up.
🤔 Maybe you were referring to this when you said that Babel can’t deal with the graphql tags yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kastermester re the issues with using babel, just wanna make sure you’ve seen this comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup - this is correct, this is sorta how our current relay classic pipeline works as well.
Ugh… all this local npm pack faffing is very cumbersome anyways :/ |
Ah correct. If the relay team has no problems with that solution then I guess it works just fine. We use babel at our work as well - but being able to remove it from some parts of our pipeline should make quite a huge impact on the performance of building the app. I'm just not too thrilled at forcing people to use some technology they might not have a need for, but then again there are other projects out there that claim to work without babel, so it is probably fine, and we can leave that until we have finished getting things to actually work. |
Alright, this is done to be merged 👍 |
I know they’d like other projects to use relay-compiler and relay-runtime, but they’ll have to accept that some other languages just don’t export CommonJS modules 🤷♂️ |
My plan was to make the following typescript transform:
const myVar = graphql`fragment MyFragment on SomeType { .. }`; into: const myVar = require('./__generated__/MyFragment.graphql.ts').default; This way none of the relay-runtime/react-relay code has to be modified. |
Indeed, not sure if there's a better way around it though? |
I think your plan for a TS plugin is a great one, if you can reduce the pipeline it‘s always a plus, imo! However, I think the baseline being that relay-compiler plugins don’t require you to write more babel transforms is a nice one nonetheless. We’ll see what they say. |
If/once we get the plugin changes merged into Relay upstream, that should already reduce the faffing by quite a lot. Then the only one that remains is the example app using the local plugin, not sure yet what a simpler solution is there. |
Well once we publish an npm package for this repo then that could also be a lot simpler I guess. |
Not sure, it’s ideal if the example app would not rely on an external built of the package so that it’s easy to immediately test changes to the plugin. |
True - perhaps some of the projects that deal with the idea of a mono repo could be of use here. It is my understanding that they can help with this sort of thing. I do not have any personal experience with that kind of setup though. |
File
object instead of just source.A few notes: