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

treat ambient non-aliased 'require' as commonjs 'require' #11819

Merged
merged 1 commit into from
Oct 24, 2016

Conversation

vladima
Copy link
Contributor

@vladima vladima commented Oct 24, 2016

fixes #11752

return resolveExternalModuleTypeByLiteral(<StringLiteral>node.arguments[0]);
}

return getReturnTypeOfSignature(signature);
}

function isCommonJsRequire(node: Node) {
Copy link
Member

Choose a reason for hiding this comment

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

May be we need to use this function in completions.ts to determine whether to give module names as completion list?

return resolveExternalModuleTypeByLiteral(<StringLiteral>node.arguments[0]);
}

return getReturnTypeOfSignature(signature);
}

function isCommonJsRequire(node: Node) {
Copy link
Member

Choose a reason for hiding this comment

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

May be we need to use this function in completions.ts to determine whether to give module names as completion list?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Member

@sheetalkamat sheetalkamat left a comment

Choose a reason for hiding this comment

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

Leave a comment

return resolveExternalModuleTypeByLiteral(<StringLiteral>node.arguments[0]);
}

return getReturnTypeOfSignature(signature);
}

function isCommonJsRequire(node: Node) {
Copy link
Member

Choose a reason for hiding this comment

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

May be we need to use this function in completions.ts to determine whether to give module names as completion list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

certainly but it is not related to the original issue so I'd put it into different PR

Copy link
Contributor

@mhegazy mhegazy left a comment

Choose a reason for hiding this comment

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

We should make sure completion is also behaving the same way.

@vladima vladima merged commit 3bdff73 into master Oct 24, 2016
@vladima vladima deleted the vladima/11752 branch October 24, 2016 23:26
@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.

commonjs module functionality is broken when project externally defined ambient require function
4 participants