Skip to content

Conversation

Gozala
Copy link
Contributor

@Gozala Gozala commented Jan 7, 2022

This pull reverts b0467e5 which reports type errors in the diff view as opposed to just dumping it into output.

It also changes TS configuration as follows:

  1. Moves tsconfig.json to the package root from src. That is more idiomatic and removes need for path mapping in tests, because TS now knows that linked multiformats package is project reference.
  2. Updates .d.ts paths to include src. As it turns out composite flag in tsconfig causes dir stracture to be retained & that flag is necessary to enable project references. We do not however generate types for tests though because those aren't included.
  3. Github action now points to test/tsconfig.json to type check everything. Because that tsconfig.json references one in the repo root type check will run over src as well.

@Gozala Gozala requested a review from rvagg January 7, 2022 19:55
package.json Outdated
"test:esm:browser": "polendina --page --worker --serviceworker --cleanup dist/esm/browser-test/test-*.js",
"test:ts": "npm run build:types && npm run test --prefix test/ts-use",
"test": "npm run lint && npm run test:node && npm run test:esm && npm run test:ts",
"typecheck": "tsc --build test/tsconfig.json",
Copy link
Member

Choose a reason for hiding this comment

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

Does it get run by the gozala/typescript-error-reporter-action? If not, then I don't think this gets run by anything automatically, or manually unless someone runs it directly, is it?

Let's wire this up to one of the main scripts that devs will be using- maybe just add it in to "test", "build:types" would also work but it's not really a "build" script per se.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just mostly added for convenience so I could run npm run typecheck, it should not create build artifacts because tsconfig.json has noEmit. I wish there was tsc check but sadly TS is not exactly what some would wish for.

I can remove this

@rvagg
Copy link
Member

rvagg commented Jan 10, 2022

Updates .d.ts paths to include src

This is fine by me but I just didn't really want to take responsibility for changing the complicated layout we have since it's getting quite complex and fragile!

Otherwise I just have that one question inline about "typecheck".

@Gozala
Copy link
Contributor Author

Gozala commented Jan 18, 2022

This is fine by me but I just didn't really want to take responsibility for changing the complicated layout we have since it's getting quite complex and fragile!

I wish there was simpler way to accomplish this. Never the less I think this does simplify config(s) which I hope should make things a bit less fragile, even if it introduces one extra directory because it removes opportunity from a random file to reshape directory layout again in the future.

From the comment I assume it is ok to land, so I'm going to proceed as I would like to submit another PR and would like to avoid rebasing things over.

@Gozala Gozala merged commit c936a6d into master Jan 18, 2022
@Gozala Gozala deleted the chore/tsc-action branch January 18, 2022 21:30
github-actions bot pushed a commit that referenced this pull request Jan 18, 2022
### [9.5.9](v9.5.8...v9.5.9) (2022-01-18)

### Trivial Changes

* Reanable tsc action and reconfigure project slightly ([#157](#157)) ([c936a6d](c936a6d))
@github-actions
Copy link

🎉 This PR is included in version 9.5.9 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants