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

External module resolution #3325

Closed
wants to merge 4 commits into from
Closed

External module resolution #3325

wants to merge 4 commits into from

Conversation

MicahZoltu
Copy link
Contributor

Fixes #2338 ... at least half of it.

TL;DR: See last commit, ignore others.

Note: This is branched off of #3324 and has Microsoft/TypeScript#externalModuleResolution rebased on master underneath it since that seemed to be relevant for this change. It also has some jake commands added for ease of testing. If this PR nears approval, I will clean it up as desired by maintainers. In the meantime, check the last commit for the meat of the PR.

Summary

Adds support for module resolution via a mapping file.

This is a proof of concept for module resolution via mapping file(s). Includes limited test coverage, no caching and poor factorization. If the spec for this feature lands on something near this PR I don't mind fixing it up, though I don't know how to write idiomatic TSC code so the PR will still likely need some maintainer love.

CAVEAT

Currently only supports references within the project root directory tree. Since dependencies managed by a package manager are likely to sit outside of the project root directory tree this solution falls short of achieving full desired results. Unfortunately, I am not sure how to find a mapping file that is shipped with third party modules (outside the project root tree) since I can't walk up the directory tree indefinitely, and it is undefined how far up is a reasonable distance to walk.

This is particularly important for dependencies that have dependencies since one can reference a dependency outside of the tree with a mapping file but that dependency can't have a mapping file of its own up its directory tree. This means that dependencies would have to either have relative file paths (no mappings) or they would need to use whatever you have defined in your project's mapping file(s). In the first case, this hurts library distribution because dependency managers have to do the terrible NPM thing of having every dependency have its own node_modules folder. In the second case, this just leads to broken apps when you cause a dependency to use a mis-matched version of one of its dependencies.

MicahZoltu and others added 4 commits May 31, 2015 15:45
Proof of concept for module resolution via mapping file(s).  Includes limited test coverage, no caching, and generally pretty crappy code.  If the spec for this feature lands on something near this PR I don't mind fixing it up, though I don't know how to write idiomatic TSC code so the PR will still likely need some maintainer love.

NOTE: Currently only supports references within the project root directory.  Since dependencies managed by a package manager are likely to sit *outside* of the project root directory this solution falls apart a little bit.  Unfortunately, I am not sure how to find a mapping file that is shipped with third party modules.  This is particularly important for dependencies that have dependencies.
@msftclas
Copy link

msftclas commented Jun 1, 2015

Hi @Zoltu, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

@msftclas
Copy link

msftclas commented Jun 1, 2015

@Zoltu, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

@mhegazy
Copy link
Contributor

mhegazy commented Jun 5, 2015

@Zoltu my apologies for the delays. @vladima is the best one to review this change as he has been looking into this for a while. but he has been pegged by a few perf and memory issues that he is investigating. i believe next week he should be able to get back to you. again sorry for the delay.

@mhegazy mhegazy assigned mhegazy and unassigned mhegazy Jun 24, 2015
@mhegazy
Copy link
Contributor

mhegazy commented Aug 31, 2015

Sorry for the delay. this should be now handles by #3147, #4154, and #4352. The change was mainly on the tooling side of the house, with VS and tsserver needing to handle resolution correctly; the previous desing had resolution responsibility split between the compiler/LS and the LS host (VS/tsserver). so that made it a bit hard to collaborate on this change. With TS 1.6, @vladima has consolidated all module resolution logic in the program, so there should be minimal host interaction into resolution, and in the future we should be able to accept similar changes without any work on the host side. Thanks again and sorry for the delay.

@mhegazy mhegazy closed this Aug 31, 2015
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
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.

External module resolution logic
5 participants