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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: enable declarationMap, use .d.ts files in monorepo too #2613

Closed
wants to merge 1 commit into from

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented Mar 19, 2019

Configure TSC to emit declaration maps, providing information needed by IDEs like VSCode to jump from usage of a symbol to its definition in the original source code.

Simplify layout of all packages, remove top-level index.ts and index.d.ts files in favor of a package.json entry

"types": "dist/index.d.ts"

Simplify layout of module packages (excluding example applications) and remove the top-level index.js file in favor of a package.json entry
"main": "dist/index.js"

Update developer documentation and project templates accordingly.

Benefits of these changes

  • It's a preparation step for TypeScript Project References, see chore: add script to build typescript project references聽#1636

  • Build is consuming less CPU cycles now. User time on my machine used to be 5m21 before, it's 3m13 now.

    UPDATE: With lib check disabled (see build: enable "skipLibCheck", remove "dom" library聽#2621), the new CPU 1m48s now and the real time is 0m37s.

    • Simpler project setup in our packages, we no longer need to maintain
      dummy index files at the top level.

    Drawbacks:

    • Refactor-rename does not work across packagas. I believe this is a
      limitation of TypeScript, hopefully it will be eventually fixed.

    • Navigation across packages may not work until the monorepo is built.

  • Simpler project setup in our packages, we no longer need to maintain dummy index files at the top level.

Drawbacks

  • The build is taking slightly more time now. Real time went from 0m49s to 0m56s. This should improve once we enable project references and incremental or watch build.

  • Refactor-rename does not work across packagas. I believe this is a limitation of TypeScript, hopefully it will be eventually fixed.

  • Navigation across packages may not work until the monorepo is built.

  • When compiling a class extending from a base class created through mixins, TypeScript emits type definitions that are violating "noImplicitAny" rule.

    declare const TodoListApplication_base;
    export declare class TodoListApplication extends TodoListApplication_base {
        constructor(options?: ApplicationConfig);
    }

    As a result, when our benchmark includes TodoApplication, the build fails. This problem does not seem to be present when Project References are enabled. As a temporal workaround, the Todo example is exporting src/index.ts for the type definitions.

See also #2609

Checklist

馃憠 Read and sign the CLA (Contributor License Agreement) 馃憟

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

@bajtos bajtos added feature Internal Tooling Issues related to our tooling and monorepo infrastructore labels Mar 19, 2019
@bajtos bajtos self-assigned this Mar 19, 2019
@bajtos bajtos requested review from raymondfeng and a team March 19, 2019 08:56
@bajtos
Copy link
Member Author

bajtos commented Mar 21, 2019

As a result, when our benchmark includes TodoApplication, the build fails. This problem does not seem to be present when Project References are enabled. As a temporal workaround, the Todo example is exporting src/index.ts for the type definitions.

I managed to solve this problem by enabling noLibCheck compiler options. See #2621

Build is consuming less CPU cycles now. User time on my machine used to be 5m21 before, it's 3m13 now.
The build is taking slightly more time now. Real time went from 0m49s to 0m56s. This should improve once we enable project references and incremental or watch build.

With noLibCheck in place, the compile time is actually faster than it used to be before.

real	0m36.980s
user	1m48.239s
sys	0m8.529s

@raymondfeng
Copy link
Contributor

The build is taking slightly more time now. Real time went from 0m49s to 0m56s. This should improve once we enable project references and incremental or watch build.

I can live with it.

Refactor-rename does not work across packagas. I believe this is a limitation of TypeScript, hopefully it will be eventually fixed.

Hmm, this will be annoying.

Navigation across packages may not work until the monorepo is built.

Understandable.

When compiling a class extending from a base class created through mixins, TypeScript emits type definitions that are violating "noImplicitAny" rule.

declare const TodoListApplication_base;
export declare class TodoListApplication extends TodoListApplication_base {
    constructor(options?: ApplicationConfig);
}

As a result, when our benchmark includes TodoApplication, the build fails. This problem does not seem to be present when Project References are enabled. As a temporal workaround, the Todo example is exporting src/index.ts for the type definitions.

Is it fixed by #2621?

@bajtos
Copy link
Member Author

bajtos commented Mar 21, 2019

Refactor-rename does not work across packagas. I believe this is a limitation of TypeScript, hopefully it will be eventually fixed.

Hmm, this will be annoying.

I agree it's annoying.

As far as I understand Project References, they rely on declaration maps too, and therefore don't support cross-package renames either.

So basically we have to choose between the two options:

  • current non-standard layout & build setup, cross-package renames supported
  • project references, cross-package renames not supported

Considering how much benefits are unlocked by package references (think about --watch, --incremental, possibly moving our tests to Jest), and how infrequently we do cross-package renames, I am personally ok to give up cross-package renames. I also hope that this will be eventually fixed in TypeScript.

Thoughts?

As a result, when our benchmark includes TodoApplication, the build fails. This problem does not seem to be present when Project References are enabled. As a temporal workaround, the Todo example is exporting src/index.ts for the type definitions.>

Is it fixed by #2621?

Yes, the error message about noImplictAny violation is fixed by #2621.

The type declaration emitted by TypeScript is still using any under the hood, loosing information about the actual methods inherited from the Application class and added by mixins. This part is unrelated to changes presented in this pull request though, I think it's a limitation (or even a bug) in TypeScript.

@raymondfeng
Copy link
Contributor

Let me play with this PR with VSCode for some time to give more concrete feedbacks.

@raymondfeng
Copy link
Contributor

Refactor-rename does not work across packagas. I believe this is a limitation of TypeScript, hopefully it will be eventually fixed.

Find all references does not work either.

@bajtos
Copy link
Member Author

bajtos commented Mar 26, 2019

@raymondfeng

Find all references does not work either.

That's pretty bad :( Do you happen to know if this problem persists with Project References too?

Configure TSC to emit declaration maps, providing information needed by
IDEs like VSCode to jump from usage of a symbol to its definition in
the original source code.

Simplify layout of all packages, remove top-level `index.ts` and
`index.d.ts` files in favor of a package.json entry

    "types": "dist/index.d.ts"

Simplify layout of module packages (excluding example applications) and
remove the top-level `index.js` file in favor of a package.json entry
    "main": "dist/index.js"

Update developer documentation and project templates accordingly.

Benefits of these changes:

- It's a preparation step for TypeScript Project References

- Build is consuming less CPU cycles now. User time used to be 5m21
  before, it's 1m48s now. Real time went down from 0m49s to 0m37s.

- Simpler project setup in our packages, we no longer need to maintain
  dummy index files at the top level.

Drawbacks:

- Refactor-rename does not work across packagas. I believe this is a
  limitation of TypeScript, hopefully it will be eventually fixed.

- Navigation across packages may not work until the monorepo is built.
@bajtos
Copy link
Member Author

bajtos commented May 10, 2019

  • Refactor-rename does not work across packages.
  • Find all references does not work either.

This is tracked by the following TypeScript issue: microsoft/TypeScript#30823

@stale
Copy link

stale bot commented May 6, 2020

This issue has been marked stale because it has not seen activity within six months. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository. This issue will be closed within 30 days of being stale.

@stale stale bot added the stale label May 6, 2020
@bajtos bajtos closed this in #5440 May 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Internal Tooling Issues related to our tooling and monorepo infrastructore stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants