-
Notifications
You must be signed in to change notification settings - Fork 25
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
Handle non @types
imports from tsconfig compilerOptions.types
#785
Handle non @types
imports from tsconfig compilerOptions.types
#785
Conversation
@@ -1,6 +1,6 @@ | |||
{ | |||
"extends": "../tsconfig.json", | |||
"compilerOptions": { | |||
"types": ["jquery", "@testing-library/jest-dom", "ignored", "@test/ignored"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why drop the ignored ones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was originally just a mistake on my part.
Looking at it again, I'm not sure if these should be silently ignored or if they should fail. They're essentially unknown dependencies, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, basically unknown dependencies.
I'd prefer keeping them to ensure we aren't changing existing behaviour. We can debate if it should error or silently ignore another time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, I think I misunderstood the original intent of including them. I didn't realize they were being explicitly ignored (# gazelle:js_ignore_imports
) 🤦♂️. It makes sense that they'd be ignored in that case.
I updated the PR to put them back, but needed to update gazelle/js/tests/tsconfig_deps/types/BUILD.in
to ignore the original, non @types
version of the import. That's still a small behavior change, but it feels right to me?
This has been merged into the internal repo and will be synced back to this repo soon. |
👍 thanks! |
…types` (#7548) Synced from #785 by walkerburgin: --- The TypeScript docs for `compilerOptions.types` make it sound like `compilerOptions.types` only applies to `@types` packages, but in practice it can also be used to include types from non `@types` in the global scope. For example, `cypress` recommends this in their [docs](https://docs.cypress.io/app/tooling/typescript-support#Configure-tsconfigjson): ```json { "compilerOptions": { "target": "es5", "lib": ["es5", "dom"], "types": ["cypress", "node"] }, "include": ["**/*.ts"] } ``` From what I can tell, if you don't specify `compilerOptions.types`, TypeScript will **not** automatically include types from a visible non `@types` package like `cypress` (`describe`, `beforeEach`, etc). But you can include non `@types` packages in `compilerOptions.types` to allow list them into the global scope. --- ### Changes are visible to end-users: no ### Test plan I updated the existing test case for `compilerOptions.types`. - Covered by existing test cases - New test cases added Closes #785 Co-authored-by: Walker Burgin <wburgin@anduril.com> GitOrigin-RevId: 4321e2b70b9e8d1f68fff219b42cae02a5d3205a
…types` (#7548) Synced from #785 by walkerburgin: --- The TypeScript docs for `compilerOptions.types` make it sound like `compilerOptions.types` only applies to `@types` packages, but in practice it can also be used to include types from non `@types` in the global scope. For example, `cypress` recommends this in their [docs](https://docs.cypress.io/app/tooling/typescript-support#Configure-tsconfigjson): ```json { "compilerOptions": { "target": "es5", "lib": ["es5", "dom"], "types": ["cypress", "node"] }, "include": ["**/*.ts"] } ``` From what I can tell, if you don't specify `compilerOptions.types`, TypeScript will **not** automatically include types from a visible non `@types` package like `cypress` (`describe`, `beforeEach`, etc). But you can include non `@types` packages in `compilerOptions.types` to allow list them into the global scope. --- ### Changes are visible to end-users: no ### Test plan I updated the existing test case for `compilerOptions.types`. - Covered by existing test cases - New test cases added Closes #785 Co-authored-by: Walker Burgin <wburgin@anduril.com> GitOrigin-RevId: 4321e2b70b9e8d1f68fff219b42cae02a5d3205a
…types` (#7548) Synced from #785 by walkerburgin: --- The TypeScript docs for `compilerOptions.types` make it sound like `compilerOptions.types` only applies to `@types` packages, but in practice it can also be used to include types from non `@types` in the global scope. For example, `cypress` recommends this in their [docs](https://docs.cypress.io/app/tooling/typescript-support#Configure-tsconfigjson): ```json { "compilerOptions": { "target": "es5", "lib": ["es5", "dom"], "types": ["cypress", "node"] }, "include": ["**/*.ts"] } ``` From what I can tell, if you don't specify `compilerOptions.types`, TypeScript will **not** automatically include types from a visible non `@types` package like `cypress` (`describe`, `beforeEach`, etc). But you can include non `@types` packages in `compilerOptions.types` to allow list them into the global scope. --- ### Changes are visible to end-users: no ### Test plan I updated the existing test case for `compilerOptions.types`. - Covered by existing test cases - New test cases added Closes #785 Co-authored-by: Walker Burgin <wburgin@anduril.com> GitOrigin-RevId: 4321e2b70b9e8d1f68fff219b42cae02a5d3205a
…types` (#7548) Synced from #785 by walkerburgin: --- The TypeScript docs for `compilerOptions.types` make it sound like `compilerOptions.types` only applies to `@types` packages, but in practice it can also be used to include types from non `@types` in the global scope. For example, `cypress` recommends this in their [docs](https://docs.cypress.io/app/tooling/typescript-support#Configure-tsconfigjson): ```json { "compilerOptions": { "target": "es5", "lib": ["es5", "dom"], "types": ["cypress", "node"] }, "include": ["**/*.ts"] } ``` From what I can tell, if you don't specify `compilerOptions.types`, TypeScript will **not** automatically include types from a visible non `@types` package like `cypress` (`describe`, `beforeEach`, etc). But you can include non `@types` packages in `compilerOptions.types` to allow list them into the global scope. --- ### Changes are visible to end-users: no ### Test plan I updated the existing test case for `compilerOptions.types`. - Covered by existing test cases - New test cases added Closes #785 Co-authored-by: Walker Burgin <wburgin@anduril.com> GitOrigin-RevId: 4321e2b70b9e8d1f68fff219b42cae02a5d3205a
The TypeScript docs for
compilerOptions.types
make it sound likecompilerOptions.types
only applies to@types
packages, but in practice it can also be used to include types from non@types
in the global scope. For example,cypress
recommends this in their docs:From what I can tell, if you don't specify
compilerOptions.types
, TypeScript will not automatically include types from a visible non@types
package likecypress
(describe
,beforeEach
, etc). But you can include non@types
packages incompilerOptions.types
to allow list them into the global scope.Changes are visible to end-users: yes
Test plan
I updated the existing test case for
compilerOptions.types
.