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

How to get all TS keywords? #208

Open
Maxim-Mazurok opened this issue May 29, 2020 · 11 comments
Open

How to get all TS keywords? #208

Maxim-Mazurok opened this issue May 29, 2020 · 11 comments
Labels
enhancement New feature or request help wanted Extra attention is needed investigation Needs to be investigated

Comments

@Maxim-Mazurok
Copy link
Owner

Related to #206

@Maxim-Mazurok Maxim-Mazurok added enhancement New feature or request investigation Needs to be investigated labels May 29, 2020
@HoldYourWaffle
Copy link

HoldYourWaffle commented Aug 1, 2020

I think all keywords are listed in the SyntaxKind enum inside the type definitions for the TypeScript compiler API. As far as I can tell, they conveniently all have the suffix 'Keyword'.
Writing a program to extract these keywords as something like a string-array should be trivial using the compiler API, which conveniently comes with its own type definitions 😉

@Maxim-Mazurok
Copy link
Owner Author

@HoldYourWaffle thanks for the tip! However, we also need to include other "keywords", such as utility type Record. See related issue #206

We definitely can extract them from either source code, or type definitions, we just have to compile a full list of such keywords that may conflict with generated interfaces.

If you'd like to help with this issue, it would be greatly appreciated :)

@Maxim-Mazurok Maxim-Mazurok added the help wanted Extra attention is needed label Aug 2, 2020
@HoldYourWaffle
Copy link

Ah, I see. Since that issue was closed I assumed it was not a problem anymore.
It shouldn't be that hard to extract those type names as well, although it's no longer trivial.

However, after some investigation I'm struggling to understand in what situations this would be an issue.
gapi's types are not declared in the global scope, so they shouldn't interfere with built-in (globally defined) types. The reason for #206 is that the global Record is hidden within the namespace where Google's Record was defined, i.e. everywhere inside (but also only inside) gapi.client.chromeuxreport.

As far as I know, this only causes issues if a Google-type wants to access a TypeScript-type that happens to have a Google-type with the same name in the same namespace, which is what happened with #211.
Based on #211, the usage of global-Record is hardcoded (which makes sense I guess, given that Google's discovery documents only return object).
Are there more of these 'hardcoded global type references'? As far as I know they won't come from the Discovery documents by themselves.

As for keywords, I doubt they're going to be an issue.
Since keywords are all-lowercase they shouldn't conflict with interface definitions.
Keywords also don't apply to property definitions (no matter what Github's syntax highlighting thinks).
This is completely legal for example:

interface MySpecialObject {
	property: {
		void: string
	}
}

Lastly, TypeScript only rejects function declarations with a keyword-name if this keyword is also a keyword in plain JS:

namespace MySpecialNamespace {
	// Perfectly fine, 'namespace' is not a JS keyword
	function namespace(): void;

	// Definitely not fine, 'while' is a JS keyword
	function while(): void;
}

These function-names should not exist in gapi, because they can't be defined in JS.

However, if my assessment turns out to be incorrect (which wouldn't surprise me given my limited knowledge on this project), I'd definitely want to help pull these 'globals' from the built-in type definitions :)

@Maxim-Mazurok
Copy link
Owner Author

Thank you, @HoldYourWaffle, you're absolutely right.

The issue was that we were using Record<> to regenerate code. And it only presented itself when google started to use it as an interface name.

This means that not everything is fine with this project (yet).
Ideally, we should prevent such things from happening.
For example, we can create a test that verifies that we're not using Record<> and other utility types in generated types. This will make it easier to maintain the project. Lowercase keywords don't matter indeed.
So, by all means, if you feel like pulling utility types from TS - that would be helpful.
In order to test, we have to run the generator first to get type definitions.
Then, we can use either regular expressions or parse AST to verify that generated types don't contain utility types. Let me know what you think about it, I'll be happy to help.

Also, our formatPropertyName() function doesn't handle ? in property name, which may result in

interface AddChartResponse {
    /** The newly added chart. */
    embedded?Chart?: EmbeddedChart;
}

But I'll move this to a separate issue.

@HoldYourWaffle
Copy link

Parsing the AST of the "included" declaration files to pull out global interface names shouldn't be that hard, even though the API can be really intuitive at times. However, I think I have enough (slightly traumatic) experience with the API to make this a not-really-trivial-but-also-not-too-bad task.

Assuming we get this list of 'global interfaces', what would we do with it to prevent conflicts? As I mentioned earlier, conflicts only occur if a Google-type references a global-type that has a Google-type with the same name in the same namespace.

I'm not a fan of renaming the Google-types, as this would create type references like gapi.client.chromeuxreport.GoogleRecord. The namespace(s) always have to be included with the type references (since you can't "import from namespace"), so this would create a lot of redundancy.

Unfortunately a global::-like modifier for type references was rejected, it would've been the perfect solution here.

@Maxim-Mazurok
Copy link
Owner Author

Maxim-Mazurok commented Aug 4, 2020

So far, the policy of preventing such conflicts was simple: inline utility types (as you can see in #211).
I'm not a huge fan of it for obvious reasons, so if you have ideas on how else we could solve this - I'm all ears.

The one thing that is important to note here, as I suspect that it creates a bit of confusion: we're not getting any code from Google. Meaning, that we will never get something that contains utility types (example). Usage of utility types was totally up to us. So, having a test will prevent us from using utility types ever again, by accident. For example, if someone makes PR that uses utility types, CI will fail, reminding us that they are potentially dangerous, as they may interfere with Google interface names.

Assuming we get this list of 'global interfaces', what would we do with it to prevent conflicts?

Ban all of them from generated types (as a precautionary measure).

Banning probably should happen in auto-generate.yml, after types are generated.
We have lint.ts script that runs dtslint on each type. Perhaps, you can run this test before/after running dtslint. I would prefer "fail fast" approach. So, if running your test takes significantly less time than dtslint, I would run your test on all types first.

UPDATE
Banning should happen in ci-test.yml, not in auto-generate.yml. ci-test will run on PRs to test them and it includes generating types in order to lint them. While auto-generate will usually run on cron to re-generate types, so it's not the best place for tests.

@HoldYourWaffle
Copy link

HoldYourWaffle commented Aug 6, 2020

I think the only way to avoid carpet-banning utility types would be to generate type definitions in a true module-format, rather than using semi-global namespaces.

This would allow us to define type aliases to TypeScripts utility types before a Google's type can hide them, without polluting the global scope. It would also most likely result in nicer import syntax, shorter type references, less confusing usage and less friction when using tooling such as webpack.

There is a big downside though: it would be a massive breaking change, the scale varying depending on how we would exactly define these modules.

I'll see if I can draft up some ways we could structure the definitions as true modules, preferably with the least amount of 'breakage' possible of course.

@Maxim-Mazurok
Copy link
Owner Author

Maxim-Mazurok commented Aug 8, 2020

It would also most likely result in nicer import syntax, shorter type references, less confusing usage and less friction when using tooling such as webpack.

Do you mind elaborating a bit on each of these points?

nicer import syntax

There's none currently since types are global.

shorter type references

You mean getting rid of /// <reference types="gapi.client" /> in @types/gapi.client.*?

less confusing usage

Really interested in what you found confusing. It'll be great if we could make @types/gapi.client.* usage less confusing.

less friction when using tools such as webpack

I didn't use @types/gapi.client.* with webpack directly, only with react-scripts. What exactly causes friction? Will be happy to lessen it.

@HoldYourWaffle
Copy link

Do you mind elaborating a bit on each of these points?

Of course not.

import syntax, shorter type references

My initial thought process went somewhere along the lines of "If we abolish namespaces completely, we could do things like import { getClassrooms } from 'gapi.client.classroom.v1' (or something along those lines)", but when I looked at the definitions again I remembered that that doesn't work, because we're not actually changing gapi itself.

In other words: big dumb from my side.

You mean getting rid of /// in @types/gapi.client.*?

I guess that's an added benefit.

less confusing usage

There's none currently since types are global.

There's a good chance that this is caused by my setup, but types weren't automatically loaded for me when using Vue single-file-components (which uses webpack). But I think you're right, the types should be global, and I shouldn't need to import them if they are. Yet I had to, although I should probably check if this wasn't caused by some configuration issue on my side.

What exactly causes friction? Will be happy to lessen it.

If I remember correctly (currently melting in a heatwave), I meant two things with 'friction' here:

  1. Having to use 'awkward' imports (for each "module"), which should not have been necessary in the first place. Could be caused either by webpack or by my own configuration (including Vue SFC's), although I think it's the latter.
  2. Having to declare 'gapi' and the likes as "externals", although that probably wouldn't be solved with a module-based definition either. Not sure what I was thinking here.

@Maxim-Mazurok
Copy link
Owner Author

Thank you for your feedback!
If you could share your Vue & webpack setup - I will be happy to take a look at it. From what I've found regarding webpack externals and typescript, this should be solvable using tsconfig.
Either way, it would be good to mention our findings in the readme.
If your project is closed source - perhaps you can strip it down and publish so that I can take a look?
Thanks again for your cooperation.

@Maxim-Mazurok
Copy link
Owner Author

Hello @HoldYourWaffle! It's been a while since the last time we talked :)
I was hoping to get that reproduction where you had to import google api types explicitly.
Please let me know if you still have the code or if you're still using google apis... Looking forward to hearing back from you, cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed investigation Needs to be investigated
Projects
None yet
Development

No branches or pull requests

2 participants