Fix the TS used for the GB-Mobile project. #21324
Conversation
SergioEstevao
added
the
Mobile App - i.e. Android or iOS
|
Size Change: +390 B (0%) Total Size: 884 kB
ℹ️ View Unchanged
|
|
I don't think this is what we'd want, since it would prevent TypeScript from being able to use types which are shipped with a package. With these changes, only community-maintained types from DefinitelyTyped would be used. The original package should always be the preferred source of types, with DefinitelyTyped serving as a fallback, often in cases where the upstream package does not ship with their own (like was the case with every |
|
Oh! I see what you're referring to as far as finding types in parent directories. While my previous point does still stand, and we don't want to be just finding types from I seem to recall having a similar conversation about types or modules being found in parent directories, and something about it "working as intended", so it would be worth finding some clarity on this point. |
|
@aduth I'm open to any suggestions... |
|
@aduth so the documentation says by default:
So this will limit it to one level instead of going up the folder no? |
|
My initial thinking is that something like this would work: {
"typeRoots": [
"./node_modules",
"./node_modules/@types"
]
}I'm still a bit unclear:
|
|
We can also do this |
|
@aduth tested your suggested options and it doesn't work, it fails with TS errors. My second suggestion also does not work. Do we have another |
|
This part of config always leaves me confused, but in general I'd expect this PR to be fine.
I don't believe that's the case. When importing a package, TypeScript will still find its types if they're included. This has to do more with automatically included types. It would be good to verify this one way or another. Redux is an example of a package that includes its types, we could test that out with this PR. I did mention to @SergioEstevao via another channel that we might reconsider the setup for Gutenberg Mobile, which I'm completely unfamiliar with. I left some relevant thoughts in a similar situation here: #18531 (comment) |
|
@aduth the documentation also says:
So is this configuration only limit were to find @types package but still allow the types defined on files? |
|
@SergioEstevao I'm not sure, but based on what you've quoted there, my interpretation would be that it may even be supported to specify a |
|
An alternative is to set Any types that aren't part of the I'm sure there are pitfalls to any of these approaches but I'm not familiar with them. As long as we're not adding anything strange to the |
I tried this but then the script Up until now the configuration I have in this PR is the one that makes the script build and work |
There was a problem hiding this comment.
I can't say that I really understand why it works, but at least in a brief test, this seems to be fine, including when a package ships with their own types.
Tested with the following diff:
diff --git a/packages/i18n/src/create-i18n.js b/packages/i18n/src/create-i18n.js
index 0b0b4cd182..f8c395073c 100644
--- a/packages/i18n/src/create-i18n.js
+++ b/packages/i18n/src/create-i18n.js
@@ -50,7 +50,7 @@ export const createI18n = ( initialData, initialDomain ) => {
*
* @type {Tannin}
*/
- const tannin = new Tannin( {} );
+ const tannin = new Tannin( 10 );
/**
* Merges locale data into the Tannin instance by domain. Accepts data in aError (expected):
packages/i18n/src/create-i18n.js:53:29 - error TS2345: Argument of type '10' is not assignable to parameter of type '{ [domain: string]: { [key: string]: TanninDomainMetadata | [string, string]; '': TanninDomainMetadata | [string, string]; }; }'.
53 const tannin = new Tannin( 10 );
~~
Found 1 error.
I'm not opposed to this as-is. |
This worked fine for GB-mobile too. @sirreal do you prefer to have it like that? |
With this approved, I'd say go ahead and merge as it is now. It's something that should be revisited either way. |
This reverts commit bfb8b7b.
* Fix the TS used for the GB-Mobile project. Make sure only types inside node_modules/@types are used. This avoids TS to find the ../node_modules/@types inside the gb-mobile repo that lives one path level up of gutenberg root. * Framework: Fix configuration spacing * Change to types node. * Revert "Change to types node." This reverts commit bfb8b7b. Co-authored-by: Andrew Duthie <andrew@andrewduthie.com>
* Add pre to list of block wrappers (#21255) * Add pre to list of block wrappers * Share the elements definition between web and native. * Make constant name all uppercase. * Rename constant name to be all uppercase * Rename elements constant to all uppercase * Fix the TS used for the GB-Mobile project. (#21324) * Fix the TS used for the GB-Mobile project. Make sure only types inside node_modules/@types are used. This avoids TS to find the ../node_modules/@types inside the gb-mobile repo that lives one path level up of gutenberg root. * Framework: Fix configuration spacing * Change to types node. * Revert "Change to types node." This reverts commit bfb8b7b. Co-authored-by: Andrew Duthie <andrew@andrewduthie.com> * [RNMobile] Dark mode on Android put on the v1.25.0 native release (#21352) * Revert revert of dark mode impl * Add pre to list of block wrappers (#21255) * Add pre to list of block wrappers * Share the elements definition between web and native. * Make constant name all uppercase. * Rename constant name to be all uppercase * Rename elements constant to all uppercase Co-authored-by: Marko Savic <savicmarko1985@gmail.com> Co-authored-by: Sérgio Estêvão <sergioestevao@gmail.com> Co-authored-by: Andrew Duthie <andrew@andrewduthie.com> Co-authored-by: Stefanos Togoulidis <stefanostogoulidis@gmail.com> Co-authored-by: Marko Savic <savicmarko1985@gmail.com>
Description
Make sure only types inside node_modules/@types are used.
This avoids TS to find the ../node_modules/@types inside the gb-mobile
repo that lives one path level up of gutenberg root.
@sirreal does this have impact on the Gutenberg TS checks?
How has this been tested?
Screenshots
Types of changes
Checklist: