meteor / babel

Babel wrapper package for use with Meteor
Other
46 stars 20 forks source link

Add support for decorators and metadata reflection in TS #28

Closed ardatan closed 4 years ago

ardatan commented 4 years ago

This PR adds decorators and metadata reflection support to TypeScript package. It doesn't affect existing users. So, libraries and frameworks like Angular, TypeORM and TypeGraphQL can be used with their decorators with the benefit of type reflection.

Suggestion: I think using babel-typescript is better than having multiple compilation process. So instead of dealing with tsconfig. People can use babelrc for enhancing TypeScript support with decorators and stuff. But this would need to remove flow plugins for TS files only due to the conflict of typing syntax.

CLAassistant commented 4 years ago

CLA assistant check
All committers have signed the CLA.

filipenevola commented 4 years ago

Hi @ardatan, I think your change broke the test.

1) meteor-babel
       can compile TypeScript with import/export syntax:
     AssertionError [ERR_ASSERTION]: Expected values to be loosely deep-equal:

is that right? Can you fix the test?

ardatan commented 4 years ago

I think tests are already failing before :) you can check master https://travis-ci.org/meteor/babel/jobs/617323220?utm_medium=notification&utm_source=github_status

ardatan commented 4 years ago

@benjamn @filipenevola Any updates?

afrokick commented 4 years ago

ping @benjamn @filipenevola

benjamn commented 4 years ago

@ardatan I'm open to removing @babel/plugin-syntax-flow and @babel/plugin-transform-flow-strip-types, as they can still be added via .babelrc, and I've never encountered anyone who relies on them in a Meteor application, compared to the popularity of TypeScript.

If we do that, is the .babelrc approach adequate to achieve the goals of this PR? Is it still necessary/useful for the TypeScript compiler to generate the metadata?

I'm not comfortable enabling decorators by default in the ecmascript package (which uses babel-compiler) until they've reached stage 3 (currently stuck at stage 2). Enabling decorators in the typescript package seems more reasonable, but there's still a pretty big risk of the syntax changing yet again in incompatible ways before stage 3, requiring application code to be rewritten, so we'd be piggybacking on the TypeScript team's acceptance of that risk.

afrokick commented 4 years ago

@benjamn How can we explicitly enable some properties? Maybe we can extend compilerOptions via packages.json?

afrokick commented 4 years ago

@ardatan do you have a workaround? I have no idea how to switch meteor-babel to my fork to support custom flags in compilerOptions

ardatan commented 4 years ago

@benjamn So removing flow would allow people to choose flow, babel-typescript or tsc. And in typescript package, it would be better to respect tsconfig.json file or at least enable those two options I added.

afrokick commented 4 years ago

This is our build log with adornis:

| Top leaves:
| plugin adornis:typescript..............................476,625 ms
| Babel.compile...........................................30,378 ms

It is official meteor's TS:

| Top leaves:
| Babel.compile...........................................56,622 ms

With official TS we had x8-x9 better results.

@benjamn please, give us a solution to customize compilerOptions via tsconfig.

hexsprite commented 4 years ago

From a developer experience perspective it would be least surprising if the tsconfig.json file was read. Those values can be merged into the defaults and if the defaults override any of the requested settings a warning can be printed. The documentation can list the default values and which are not overridable and why.

menewman commented 4 years ago

Re: this comment --

I'm open to removing @babel/plugin-syntax-flow and @babel/plugin-transform-flow-strip-types, as they can still be added via .babelrc, and I've never encountered anyone who relies on them in a Meteor application, compared to the popularity of TypeScript.

For what it's worth, we've been using Flow + Meteor in our production application (CodeSignal) for years, and part of the original choice to choose Flow over TypeScript was Meteor's built-in support for Flow at the time (back when we were looking at options in ~2017).

No complaint here about removing those packages, regardless -- as mentioned, it's no big deal for us to add the Babel Flow plugins back in ourselves as we upgrade meteor-babel. Just wanted to chime in and say that someone out there actually is using Flow instead of TypeScript, in case that's relevant when making any future decisions. 😅 The reason I ended up in this thread is because I was verifying why our build started breaking after doing a Meteor update, since this wasn't in the Meteor changelog as a breaking change.

hexsprite commented 4 years ago

@menewman I also got bit by this. The breaking change was in 1.10.2, actually. It was in the release notes. But it looks like that page was not updated yet. Would be nice if it was automatically synced with what is released...

https://github.com/meteor/meteor/blob/devel/History.md#v1102-2020-04-21

menewman commented 4 years ago

Thanks for pointing out that it's in the History file, @hexsprite! I have noticed that sometimes the changelog lags behind History.md, as you said. I'm glad to see that removal of built-in Flow support is in fact listed there as a breaking change. 👍

hexsprite commented 4 years ago

I believe what's holding up this PR is that it should read the tsconfig.json and parse out the settings instead of forcing all users to have decorators + etc enabled by default. I suggest to whitelist which properties should be parsed out of the tsconfig.json and that if there are any other properties in the tsconfig.json that attempt to override Meteor's typescript compiler default values that a warning is printed with a reference to consult the Meteor typescript docs which can explain which props are allowed.

davidworkman9 commented 4 years ago

Does anyone know if there is any way I can make my app use a local version of this package that has these changes? (I'm not even sure what package this is, as there's no package.js file in this repo)

ardatan commented 4 years ago

@benjamn @filipenevola Any thoughts?

filipenevola commented 4 years ago

Closed in favor of https://github.com/meteor/babel/pull/34