Skip to content
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

Ensure our project references include transitive references #55339

Closed
wants to merge 5 commits into from

Conversation

jakebailey
Copy link
Member

Without #30608, there's a chance that building a single project may not actually cause another to be invalidated, so we must explicitly verify that all are listed.

I also added a script which says when something's wrong.

Pulled out of #55273 (comment) since this change is unrelated.

@andrewbranch
Copy link
Member

Sheetal is the expert here, but this doesn’t seem right to me. In both of the referenced issues, users were trying to take a direct dependency on a transitive reference and were surprised that it doesn’t redirect the reference or invalidate the build when the transitively referenced project had changes. That’s not what we have. tsserverlibrary/tsconfig.json doesn’t need to reference deprecatedCompat or typingsInstallerCore because it doesn’t directly import those things. To quote myself from one of the referenced issues:

the way I have always set up project references is if I import directly from another project (as you imported projectC from projectA in your example), I list projectC as a reference of projectA. On the other hand, if I use exports from projectC only indirectly through projectB, I only list projectB as a reference of projectA.

@jakebailey
Copy link
Member Author

That's how it feels like it should work, yeah, though #55273 (comment) seemed to imply otherwise unless I'm misunderstanding something here.

@andrewbranch
Copy link
Member

I also think #30608 was just an ancient bug that was actually fixed?

I think I also had issues recursively compiling all nested project references, but the ---build options seem to handle that now, and Visual Studio seems to handle it fine also (perhaps I just didn't know about it then, or didn't upgrade yet).

@andrewbranch
Copy link
Member

From @sheetalkamat:

i think if you change something in deprecatedCompat, it wont build tsserver library since its not direct dependency unless server needs to be built too. (which is what may be is intended)

I think this is intended; why would it not be? If some change in deprecatedCompat doesn’t require server to rebuild, why should it require tsserverlibrary to rebuild when tsserverlibrary only depends on deprecatedCompat through server, which is unchanged?

@jakebailey
Copy link
Member Author

Yeah, I guess feasibly I can just not do anything except list the things that are actually imported in each project (what I did originally) and be fine?

@jakebailey jakebailey marked this pull request as draft August 11, 2023 22:18
@andrewbranch
Copy link
Member

Aren't we protected from accidentally importing something external without including it in references because it will trigger the “all inputs must be within rootDir” error?

@jakebailey
Copy link
Member Author

Yes, though this PR in particular is about the opposite direction of referencing things that are unused within the project itself.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Looks pretty good with a few suggestions

.github/workflows/ci.yml Outdated Show resolved Hide resolved
scripts/checkProjectReferences.mjs Show resolved Hide resolved
scripts/checkProjectReferences.mjs Outdated Show resolved Hide resolved
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

As others point out, I don't know if this is needed, but the code itself looks OK.


/**
* @param {string} p
* @param {Set<string>} seen
Copy link
Member

Choose a reason for hiding this comment

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

minor quibble: there's no reason to pass seen around instead of referring to it from inside getReferencesWorker

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I had it like that before but switched it to this. I'm not picky but I'm too not sure if this PR is still needed.

@jakebailey
Copy link
Member Author

I'll just close this; if it turns into a problem again we can revive it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants