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

Use typescript 3.7 to emit declarations from js code, Use composite build #3805

Closed
wants to merge 1 commit into from

Conversation

Bnaya
Copy link
Contributor

@Bnaya Bnaya commented Oct 11, 2019

Using composite project and the new ability of ts 3.7 of allowJs + declarations.

  • very good DX & smarter build
  • No need to run build for dependencies to edit code in editor
  • lets us build d.ts files for also the js based packages, so we may bundle types with all of the
    packages.
    * Typescript 3.7 is not released yet, but there are no errors.
    Maybe the merge should wait.

I'm really not sure how good the inferred declarations are.
symbols discoverability should be good, and that's a big plus,
but maybe using React components without clear props will confuse developers more than help them.
So we can choose to not ship the declarations inside the bundle.

Improving the types will not require full ts refactor, it can be done incrementally, using checkJs on the relevant package and adding typescript-jsdoc types.

The entry point for all of the typescript programs is tsconfig.build-all.all.json
That reference all of the inner tsconfigs, in cjs & esm variant.
means, cjs & esm are built as part of one big project. no need a separate invocation

Development workflow using:

yarn tsc --build tsconfig.build-all.all.json --incremental --watch

Will give very efficient dev workflow, while watching the entire codebase, the incremental option makes you "pay" only for your changes.

You may still run tsc --build -w on specific packages, without the need to build the packages it depends on before, as it will also build it for you as composite project.

The main thing that missing: build also esm

Some of the js packages emitted d.ts are broken due to:
microsoft/TypeScript#33626

run yarn validate-emitted-declarations to validate the emitted d.ts files
I think that as first step, it's better to shipping the emitted d.ts files for the js-based packages.
I've added a workaround to emit valid d.ts, and also 33626 is expected to be fixed before 3.7 stable.

@Bnaya Bnaya force-pushed the allowjs-plus-declarations branch 2 times, most recently from 2a66894 to 9793413 Compare October 14, 2019 23:00
@Bnaya Bnaya changed the title Use typescript 3.7 to emit types from js code, Use composite build Use typescript 3.7 to emit declarations from js code, Use composite build Oct 15, 2019
@Bnaya Bnaya marked this pull request as ready for review October 15, 2019 12:36
@Bnaya Bnaya force-pushed the allowjs-plus-declarations branch 2 times, most recently from 6e11371 to db1c33c Compare October 30, 2019 07:46
@Bnaya Bnaya force-pushed the allowjs-plus-declarations branch 3 times, most recently from a715e66 to e770f53 Compare November 8, 2019 19:16
@PaulMest
Copy link
Contributor

@Bnaya does this help with import discovery and props autocompletion in VS Code? Could you record a short demo of how this works?

@Bnaya
Copy link
Contributor Author

Bnaya commented Jan 24, 2020

Hey Paul,
Yes it helps,
But do you mean record a demo?

@PaulMest
Copy link
Contributor

Or maybe at least a screen shot just to help people understand how helpful this PR would be?

IntelliJ IDEA from the latest release (not this PR's version) cannot auto-import any packages:
image

VS Code is also pretty clueless:
image

@user753
Copy link

user753 commented Jan 30, 2020

@Bnaya could you publish generated declarations for ra-ui-materialui as a package for https://github.com/DefinitelyTyped/DefinitelyTyped ?

@Bnaya
Copy link
Contributor Author

Bnaya commented Jan 31, 2020

i gave it another spin on another branch,
See example:
Screen Shot 2020-01-31 at 18 33 37

Screen Shot 2020-01-31 at 18 34 49

Unfortunately i was not able to get any attention from the project maintainers, so i'm not sure how to proceed

I'm not sure if publishing to DT would be the right corse of action and it will be additional work for me

@Bnaya
Copy link
Contributor Author

Bnaya commented Jan 31, 2020

@fzaninotto
Copy link
Member

@Bnaya you've changed the typescript and makefile configurations completely, without explaining why. To me, it looks more like a fork than a contribution. If you actually want your code to be merged, please:

  • resolve the conflicts
  • update as little config files as possible
  • explain why you made some changes

@Bnaya
Copy link
Contributor Author

Bnaya commented Jan 31, 2020

@fzaninotto
First i must say I'm really appreciate your work on that project.
I have no complaints for not getting any attention, i know your team has its prios.
I made this PR as POC + getting early feedback & collaboration from the maintainers be because its not a change that can be made with simple change of lines configs lines.

This is not a fork at all, but somewhat refactor of some of the build, as expected from this kind of change.
it's very possible some of the things can be made simpler - with your collaboration and review it will get better.

@Bnaya Bnaya force-pushed the allowjs-plus-declarations branch from e770f53 to 5b1340a Compare January 31, 2020 20:39
@Bnaya
Copy link
Contributor Author

Bnaya commented Jan 31, 2020

I will add more info:
this PR includes 2 things, that for my POV goes very good together:

  • Emit typescript declarations
  • Use typescript composite build / project references

Using typescript composite build before 3.7 wasn't possible because it requires also emitting declarations.

If this change of build is not acceptable on this time, i may try to just add declarations emit on a separate branch + pr without composite build

@Bnaya
Copy link
Contributor Author

Bnaya commented Jan 31, 2020

Declarations only change done, See PR: #4375
I'm still for switching to composite build

@user753
Copy link

user753 commented Feb 3, 2020

I'm not sure if publishing to DT would be the right corse of action

At least your work could be useful and would save many man-hours instead of sitting here for four month.

@fzaninotto
Copy link
Member

Thanks.

As I said in #4375, the first step before publish types is to migrate the demo completely to TypeScript. that's because we want to be sure that the types we publish don't break existing TS apps.

So if you want to help react-admin to have types, please start with the demo!

@Bnaya
Copy link
Contributor Author

Bnaya commented Feb 3, 2020

I'm closing this PR is favor of moving discussion to #4375
The composite build stuff will wait i guess

@Bnaya Bnaya closed this Feb 3, 2020
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.

4 participants