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

chore(build): switch to babel for compiling typescript #2341

Closed

Conversation

JoelEinbinder
Copy link
Contributor

This patch adds babel, which is used to transform our TypeScript into JavaScript. The main difference is that types are not checked during build. This removes the cyclical dependency we had where the types were required to build, but a built playwright was required to generate the types. It also makes building faster by a few seconds.

As a bonus, this exposed a bug where we were exporting some types instead of classes in api.ts. This caused Browser and BrowserContext to be excluded from the automated coverage tests.

@pavelfeldman
Copy link
Member

Wouldn't babel run the same typescript compiler with the generate-only options? Why bringing it in?

@JoelEinbinder
Copy link
Contributor Author

Wouldn't babel run the same typescript compiler with the generate-only options? Why bringing it in?

Where is the generate-only option?

@@ -15,14 +15,14 @@
*/

export { Accessibility } from './accessibility';
export { Browser } from './browser';
export { BrowserContext } from './browserContext';
export { BrowserBase as Browser } from './browser';
Copy link
Member

Choose a reason for hiding this comment

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

These don't look like improvements either

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These fix bugs in apicoverage.spec.js. Previously we were exporting a type, which got stripped by TypeScript.

@JoelEinbinder
Copy link
Contributor Author

Wouldn't babel run the same typescript compiler with the generate-only options? Why bringing it in?

Where is the generate-only option?

I think we are waiting on microsoft/TypeScript#29651

@pavelfeldman
Copy link
Member

Yeah, looks like Babel is generating code on its own there. Isn't it scary though? I'd be more comfortable with not bringing more tools into the game. What's broken? We need to ignore the errors from the first build pass? Does not sound like a big deal.

Maybe protocol generation should be a part of the roll process instead?

@JoelEinbinder
Copy link
Contributor Author

Yeah, looks like Babel is generating code on its own there. Isn't it scary though? I'd be more comfortable with not bringing more tools into the game.

Babel + TypeScript is a very standard workflow. I trust Babel just as much if not more than TypeScript to compile our code. Especially since we currently use TypeScript to convert our code to be compatible with Node 10.

What's broken? We need to ignore the errors from the first build pass? Does not sound like a big deal.

I received complaints about the current setup. We can stick with it and it will be mostly fine. I'll send a separate patch to fix the coverage.

The problem with ignoring errors is that we don't know which errors are safe to ignore. A syntax error in the code will fail silently and run the library from the previous commit.

Maybe protocol generation should be a part of the roll process instead?

I don't like committing generated files when I can help it. I find they inevitably end up out of date with the code that generates them.

@pavelfeldman
Copy link
Member

I don't like committing generated files

This is different. You commit chrome.exe and protocol is a part of it. You are right in that we should not commit files that are generated from our source, but this is not a part of our source.

@pavelfeldman
Copy link
Member

It also makes building faster by a few seconds.

In my mode, there is no building at all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants