Potential language server changes

[kastermester]

Reading through the changelog of VS Code 1.20 i found this:

Spaces got you down? When you type ., VS Code now shows all known properties for JavaScript and TypeScript, even if a property name contain whitespaces or other non-identifier characters.

It seems we need a new way to deal with hiding members other than simply prefixing a whitespace. I'm not sure if it is possible to use symbols or something similiar to accomplish this?

[kastermester]

@alloy (forgot to tag you)

[alloy]

I had indeed noticed that, but wasn’t too worried about it because at least it won’t show up when you’re typing. Nonetheless, completely hiding would be nicer indeed.

[kastermester]

Having tested this a bit, it seems we're still good for now (they auto complete for spaces and wonky characters, but not if they are leading, it's probably a bug). However we might want to look into using symbols for this.

Something like this:

File: react-relay.d.ts

export const Fragments$Symbol: unique Symbol;
export const FragmentId$Symbol: unique Symbol;

File __generated__/SomeFragment_data.graphql.ts

import { FragmentId$Symbol } from 'react-relay';

export declare const SomeFragment_data$ref: unique symbol;

export type SomeFragment_data = {
  [$FragmentId$Symbol]: typeof SomeFragment_data$ref;
  ...
}

As far as I can see this has all the same behaviors we currently get - only now we're not using fancy empty enums to achieve what the type system now handles quite well. I'll look into making a PR at some point - but perhaps we should simply do this when we fix the remaining types, seeing as this will also require people to use a somewhat recent TS version (symbols were introduced in 2.7 as far as I can see).

[kastermester]

I had indeed noticed that, but wasn’t too worried about it because at least it won’t show up when you’re typing. Nonetheless, completely hiding would be nicer indeed.

Actually property names with spaces do show up, just not when it is a leading space it seems :)

[alloy]

So symbols don’t show up? And is that documented behaviour?

I do like replacing the enums with symbols, I was also just thinking about how we’re using Babel 7 for some of our compilation needs in a web app that we’ll be using Relay Modern in soon and the Babel 7 TS parser does not allow const enum.

[kastermester]

So symbols don’t show up? And is that documented behaviour?

They don't show up, but it is, as far as I can see, not documented - but think of the consequences it would have if they showed up - type []. and suddenly a wealth of symbols would show up. We could ask the TS team about this - but I'd say it's fairly safe to say they will not show up.

I do like replacing the enums with symbols, I was also just thinking about how we’re using Babel 7 for some of our compilation needs in a web app that we’ll be using Relay Modern in soon and the Babel 7 TS parser does not allow const enum.

That is a bummer. Don't you compile TS using the TS compiler before passing it to babel though? (More or less, set target language in tsconfig to ES2015 to leave the tagged template literals in the compiled code).

On an unrelated note, i sent you an email yesterday in the email listed on your profile (not sure if you actively use that).

[alloy]

I'd say it's fairly safe to say they will not show up.

Fair enough, I think you’re right 👍

Don't you compile TS using the TS compiler before passing it to babel though?

We do right now, but in the cases where we really can’t get away from using Babel we’d prefer to optimise the performance by having 1 compiler instead of multiple.

On an unrelated note, i sent you an email yesterday in the email listed on your profile (not sure if you actively use that).

Hah, I had just replied to that previously :)

[kastermester]

We do right now, but in the cases where we really can’t get away from using Babel we’d prefer to optimise the performance by having 1 compiler instead of multiple.

Makes sense. I have done some initial testing on our own codebase, for performance considerations, what getting rid of babel for TS code would mean. We found a rough 25% of the build time was removed by getting rid of babel (babel 6.5.x). It might be worth it testing the transformation plugin up in the PR for you guys. For Relay modern I can't imagine it having any real issues.

EDIT: forgot to mention, we have a pipeline of both TS and babel currently. I have not tested what dropping TS means, though my gut feeling tells me that the TS loader is much much faster than the babel loader. This comes from ie. running our example code with the TS transformation stuff I made - which ran way faster.

[alloy]

Alas we can’t get rid of Babel that easily in our React Native codebase, as React Native itself comes as Flow annotated source (not transpiled) and its packager has many assumptions. I would definitely believe TSC to be faster, even just from the fact that people at MS are paid to work on it.

[kastermester]

Yeah TS and React Native are yet to become great friends. We have done very little work on React Native (and I haven't been that involved with it), but I do believe we're compiling from TS to JS before letting babel do its thing.

[alloy]

You mean statically? We have our own custom RN transformer that uses TSC to transpile on the fly before passing on to Babel https://github.com/artsy/emission/blob/3a415954b7b163703a5ed9fda41e7053de681469/transformer/index.js

[kastermester]

Sweet, I'll pass that along to our RN guy :) thx!

[orta]

Would recommend https://github.com/ds300/react-native-typescript-transformer which was based on our code (also has the babel feature) and generally supports more than one version of RN. I have a PR moving Emission (our RN) to it

[alloy]

Yup, good point @orta 👍

[kastermester]

Cheers! :)

I have asked around on that commit and it seems Relay will be allowing self referencing fragments in some forms in which it can be compiled/unrolled to a valid GraphQL Query. As such it seems the GraphQL spec is not changing in this aspect, but just that Relay will do its thing and make the query valid. We should probably look to incorporate those changes into this plugin as well.