Skip to content

Use project relative preference for declaration emit #42232

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

Merged
merged 4 commits into from
Apr 30, 2021

Conversation

sheetalkamat
Copy link
Member

@sheetalkamat sheetalkamat commented Jan 6, 2021

Fixes #42349

@sheetalkamat
Copy link
Member Author

@typescript-bot test this
@typescript-bot user test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 6, 2021

Heya @sheetalkamat, I've started to run the extended test suite on this PR at 85db43e. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 6, 2021

Heya @sheetalkamat, I've started to run the parallelized community code test suite on this PR at 85db43e. You can monitor the build here.

@@ -191,7 +191,9 @@ namespace ts.moduleSpecifiers {
}

if (relativePreference === RelativePreference.ExternalNonRelative) {
const projectDirectory = host.getCurrentDirectory();
const projectDirectory = compilerOptions.configFilePath ?
Copy link
Member Author

Choose a reason for hiding this comment

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

This change is so that it actually uses directory of tsconfig file and not random directory where tsc is invoked from. (eg in test added the current directory is "/")

@typescript-bot
Copy link
Collaborator

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

I added project-relative as a way to use somewhat looser heuristics for auto-import paths than we do in the compiler generally, namely the existence of package.json files in source directories, which otherwise tsc would never care about unless node module resolution for a bare specifier resolved to that directory. Does #42224 not fully solve the issue? That is a much more explainable fix, if nothing else, and seems to fit nicely with an existing ethos of reusing input nodes and paths elsewhere in the transformers/emitter.

@sheetalkamat
Copy link
Member Author

It does solve the issue but only if file was imported in the file, not otherwise. This would give use better paths in declaration emit. interesting thing is it didn't break anything so i wonder if this is better behavior for those derived paths. may be it is better option if importing file is project reference redirect?

@robpalme
Copy link

robpalme commented Jan 9, 2021

may be it is better option if importing file is project reference redirect?

Please try to avoid making declaration emit dependent on whether the source/destination crosses a boundary expressed specifically as a project reference.

Our higher-level tooling (think Lerna etc) constructs multi-project workspaces dynamically. The workspace is just a list of packages you want to develop together live - and the user adds and removes packages frequently in the course of developing or debugging an issue. Meaning a project might have no project references one minute (because it is being built in isolation and so only links to static/published declarations for dependencies via "paths"). Then the next minute the user wants to experiment by adding another project to the workspace, and so the tooling injects a project reference into tsconfig (causing the parent project to now consume declarations generated live from the child project's source code).

It would be odd if this lightweight switching action resulted in different behavior in declaration emit.

@sheetalkamat
Copy link
Member Author

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 15, 2021

Heya @sheetalkamat, I've started to run the tarball bundle task on this PR at 28d3a36. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 15, 2021

Hey @sheetalkamat, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/93415/artifacts?artifactName=tgz&fileId=8571D4CACB394041C1A919F295B6CB437DAFEB81173361A4A88885E8A6AE839602&fileName=/typescript-4.2.0-insiders.20210115.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

@sandersn
Copy link
Member

@sheetalkamat is this obsolete now that #42224 is in? Or is it worth continuing on? If so, is it ready for review? I couldn't tell from skimming the conversation.

@sheetalkamat
Copy link
Member Author

I dont think this is absolete. There is #42349 which might need this so need to investigate further and add test before this is ready for review.

@sheetalkamat sheetalkamat force-pushed the projectRefUsesNonRelativeImport branch from 28d3a36 to 9d1b450 Compare April 29, 2021 23:47
@sheetalkamat
Copy link
Member Author

@andrewbranch @weswigham i think we should take this PR as this does help many scenarios.. Eg #42349 where there are no project references or node_modules involved .. its all path mapping. I think declaration emit to use non relative module specifier if project crosses boundary is good thing rather than always using relative path.
Please review and weigh in again.
Thanks

Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

I’m not completely confident that project-relative will produce correct results 100% of the time, but I do think it will be better on average. It’s probably fine to move forward and iterate on this if we get feedback about it.

@sheetalkamat sheetalkamat merged commit 54096bd into master Apr 30, 2021
@sheetalkamat sheetalkamat deleted the projectRefUsesNonRelativeImport branch April 30, 2021 20:22
eps1lon added a commit to mui/material-ui that referenced this pull request May 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

With project references, some cross-package imports of inferred types are emitted with relative paths
5 participants