Apply the safe list to projects which didn't specify an upfront include#20048
Conversation
| return counts.ts === 0 && counts.tsx === 0; | ||
| } | ||
|
|
||
| /* @internal */ |
There was a problem hiding this comment.
Really this function should be called hasNoTypeScriptSource.
| Debug.assert(!!newTypeAcquisition, "newTypeAcquisition may not be null/undefined"); | ||
| Debug.assert(!!newTypeAcquisition.include, "newTypeAcquisition.include may not be null/undefined"); | ||
| Debug.assert(!!newTypeAcquisition.exclude, "newTypeAcquisition.exclude may not be null/undefined"); | ||
| Debug.assert(typeof newTypeAcquisition.enable === "boolean", "newTypeAcquisition.enable may not be null/undefined"); |
There was a problem hiding this comment.
If only we had a type system that could check these kind of things... 🤔
There was a problem hiding this comment.
Did you check this holds if the tsconfig.json just has "typeAcquistion": {"enable": true} (i..e include & exclude are still set to defaults).
There was a problem hiding this comment.
Isn't this code specific to external projects? It looks like configured projects have their own setTypeAcquisition.
There was a problem hiding this comment.
Right, this is in ExternalProject so there can't (?) be a tsconfig.json
billti
left a comment
billti
left a comment
There was a problem hiding this comment.
I've yet to validate it empirically, but theoretically it looks good 👍
| this.sendResponse(this.createSetTypings(req, discoverTypingsResult.cachedTypingPaths)); | ||
| // respond with whatever cached typings we have now if there are no other types left to install | ||
| if (discoverTypingsResult.newTypingNames.length === 0) { | ||
| this.sendResponse(this.createSetTypings(req, discoverTypingsResult.cachedTypingPaths)); |
There was a problem hiding this comment.
this.sendResponse(this.createSetTypings(req, discoverTypingsResult.cachedTypingPaths)); [](start = 16, length = 87)
Maybe move this into the else at the end of this method?
| for (const t of types) { | ||
| proj.typeAcquisition.include.push(t); | ||
| } | ||
|
|
If it can be null, do you have to check that before accessing Refers to: src/server/editorServices.ts:2271 in 744511c. [](commit_id = 744511c, deletion_comment = True) |
| for (const type of rule.types) { | ||
| if (types.indexOf(type) < 0) { | ||
| types.push(type); | ||
| if (typeAcqInclude.indexOf(type) < 0) { |
There was a problem hiding this comment.
if (typeAcqInclude.indexOf(type) < 0) { [](start = 32, length = 39)
We don't otherwise dedup the include or exclude lists, do we?
There was a problem hiding this comment.
Not if they're user-specified
|
👍 . Please port to release-2.6 also. |
|
Test fix incoming |
RyanCavanaugh
force-pushed
the
fixSafeList
branch
from
1f39728 to
ad6f1eb
Compare
…te project contexts when installer reqs finish
RyanCavanaugh
force-pushed
the
fixSafeList
branch
from
ad6f1eb to
a416826
Compare
Port #20048 to release-2.6
3 participants
cc @billti @amcasey