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

Allow export map entries to remap back to input files for a program #47925

Merged
merged 7 commits into from
May 5, 2022

Conversation

weswigham
Copy link
Member

Fixes #46762

As the code may show, this is more of a mire than it appears at first blush. Because our output directory structure depends on the set of files we load (unless composite or rootDir is set), there's a circular dependency here - we need to resolve our set of input files to know what the output directory structure is, but we might need to know the output directory structure to know how to find our input files. Given that, I've employed a bit of a heuristic - in module resolution, we assume the common source directory is the top-most folder between the package file/config file and the directory we're resolving in that we can find that has a file matching the name we're looking for in it. Technically, this can never be wrong, since it'll cause us to load those top-level files, which'll, in turn, guarantee that the common source directory is at that top level. It might end up being surprising that adding an export map to your package.json and using a self-name import can change your output directory structure, but in some sense it makes sense, since adding an import changes that structure, too - this import just happens to (partially) be in your package.json.

AFAIK, this circular dependence issue was technically observable prior to export maps - imports like import("../../dist/other/file.js") don't resolve (to the input source) in TS today, we just see very little to no demand for fixing that issue, whereas once you couch it in terms of package exports and self names, such redirection gets a little more desirable.

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Feb 17, 2022
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.

Conclusion from design meeting #48095 was to enforce rootDir.

@weswigham
Copy link
Member Author

@andrewbranch @DanielRosenwasser you wanna re-review this?

@andrewbranch
Copy link
Member

So basically you kept the same guessing algorithm to use as a fallback when we issue the rootDir diagnostic?

@weswigham
Copy link
Member Author

Yep.

fragment = ensureTrailingDirectorySeparator(commonDir);
}
}
if (commonSourceDirGuesses.length > 1) {
Copy link
Member

Choose a reason for hiding this comment

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

If I’m reading this right, I find it troubling that if your project is sufficiently close to the disk root, the requirement of specifying rootDir is removed. It feels like where your fully-contained project directory is on disk should not affect program diagnostics—this seems like an easy way to get a compilation that succeeds in Docker because it was mounted at / but fails on the host machine, for example. I would lean toward a simple cause and effect: using X feature requires Y config, without any “weeeell technically” clauses. It also makes me wonder how much of this complexity can be removed. Why not use a simpler and more naive guess (config file path || current directory?) in the error case and save ~100 LOC?

Copy link
Member Author

Choose a reason for hiding this comment

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

If I’m reading this right, I find it troubling that if your project is sufficiently close to the disk root, the requirement of specifying rootDir is removed.

So... Sort-of. If the common path between the requesting file and the target file is the system root, then, yes. Every guess will be the system root, so there'll only be one unique guess. (Because it's the only possible root directory)

I don't think "mount location independence" is an aspect of our module resolver that it actually has - node_modules lookups trivially falsify that already. (Since you could mount somewhere that a parent node_modules that was used for resolution folder no longer exists.)

Copy link
Member Author

@weswigham weswigham Apr 28, 2022

Choose a reason for hiding this comment

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

in the error case and save ~100 LOC?

Tbf, most of that is comments explaining why we don't actually know the final path and are making a guess. The actual guessing code is about 10 lines, changing the method wouldn't reduce it much, or even shrink the comment (just reduce how often it actually finds a file to pair with an error for better services behavior).

@weswigham
Copy link
Member Author

@DanielRosenwasser what/where/when do we want this merged?

@DanielRosenwasser
Copy link
Member

(There are conflicts now because some of these changes affect deleted baselines.)

It seems like this is part of the node16/nodenext story, so we should get it merged by today IMO.

@weswigham
Copy link
Member Author

@DanielRosenwasser updated.

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.

--module nodenext self-name references don't work with an outDir from clean build
4 participants