Skip to content

Conversation

@sheetalkamat
Copy link
Member

No description provided.

if (oldRefs) {
return oldProgram.structureIsReused = StructureIsReused.Not;
}
Debug.assert(!oldRefs);
Copy link
Member

Choose a reason for hiding this comment

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

For future readers it'd be nice to comment why this can't be the case (because getResolvedProjectReferences shouldn't return anything unless getProjectReferences did)

}

function getProjectReferences() {
function getResolvedProjectReferences() {
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this is a good change, but still it's a breaking API change and should be documented somewhere

sourceFile.resolvedTypeReferenceDirectiveNames.set(typeReferenceDirectiveName, resolvedTypeReferenceDirective);
}

export function projectReferencesIsEqualTo(oldRef: ProjectReference, newRef: ProjectReference) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider renaming to projectReferenceIsEqualTo since it only handles one reference at a time there's no need for plural in the same

@RyanCavanaugh
Copy link
Member

@ajafff had a good point - we should reshuffle the names so we don't break any existing consumers of that function

@sheetalkamat
Copy link
Member Author

@RyanCavanaugh I looked though github and I didn't find any use for existing getProjectReferences except one at https://github.com/fimbullinter/wotan/blob/79b6cf37616fc7df9a88a5ebb4211ec284a00eef/packages/wotan/src/project-host.ts#L238 which would rather do better with the new method of getProjectReferences. Is that good enough reason to accept this breaking change in API.
Otherwise can you propose what to call the getProjectReferences (that return ProjectReferences and not resolved ones) Thanks.

@ajafff
Copy link
Contributor

ajafff commented Sep 14, 2018

@sheetalkamat As I said, I think this is a good change as I wrote that line you linked to. I think this is pretty easy to feature-detect if you change the purpose of the existing method.

Besides that there is TypeStrong/ts-loader#817 which also uses the method, but could also feature-detect which one to use.

@RyanCavanaugh
Copy link
Member

Let's just go ahead and change it then

@sheetalkamat sheetalkamat merged commit e471856 into master Sep 15, 2018
@sheetalkamat sheetalkamat deleted the watchAPIAndProjectReferences branch September 15, 2018 00:58
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants