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

Use more nodelike paths for import types when possible #24610

Merged

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Jun 1, 2018

Partial fix for #24599

A complete fix (ie, one where /index is also removed and maybe where package.json's get walked backward) requires a much more complex reverse resolver to check there's no directory/file conflicts (in the case of /index) and for file contents (in the case of reversing a package.json). If we think that's worth doing, we can; but this simple change of stripping the leading relative part makes the output as portable as is usually expected.

@weswigham weswigham requested review from rbuckton and mhegazy June 1, 2018 23:29
@mhegazy
Copy link
Contributor

mhegazy commented Jun 3, 2018

We have the code to this in the services. We use it to generate import quick fixes.

@weswigham weswigham force-pushed the basic-import-path-nodification branch from 3543ffb to 052d1a2 Compare June 4, 2018 19:30
@weswigham
Copy link
Member Author

@mhegazy Moved, patched tryGetModuleNameAsNodeModule to handle file/directory conflicts appropriately (previously it would always use module/foo for module/foo/index even if module/foo.ts existed), and used.

@weswigham weswigham requested a review from a user June 4, 2018 19:32
@weswigham weswigham force-pushed the basic-import-path-nodification branch from 052d1a2 to d79d781 Compare June 4, 2018 19:33
@weswigham weswigham force-pushed the basic-import-path-nodification branch from d79d781 to ebfda15 Compare June 4, 2018 19:33
@mhegazy mhegazy requested a review from sheetalkamat June 4, 2018 20:00
@mhegazy
Copy link
Contributor

mhegazy commented Jun 4, 2018

@sheetalkamat and @andy-ms can you please review this change.

const extensions = getSupportedExtensions({ allowJs: true }, [{ extension: "node", isMixedContent: false }, { extension: "json", isMixedContent: false, scriptKind: ScriptKind.JSON }]);
for (const e of extensions) {
const fullPath = path + e;
if (host.fileExists!(fullPath)) {
Copy link

Choose a reason for hiding this comment

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

Might want to mark this with #18217

@weswigham weswigham merged commit d9b9390 into microsoft:master Jun 5, 2018
@weswigham weswigham deleted the basic-import-path-nodification branch June 5, 2018 19:54
weswigham added a commit to weswigham/TypeScript that referenced this pull request Jun 12, 2018
* Use more nodelike paths for import types when possible

* move functionality from services into compiler, fix with propert file/directory conflict handling

* mark suspect cast
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants