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

Include '@types/node' in ATA if commonjs modules are used #20844

Closed
wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Dec 21, 2017

@DanielRosenwasser had recommended doing this.

@@ -3968,6 +3968,7 @@ namespace ts {
typeAcquisition: TypeAcquisition; // Used to customize the type acquisition process
compilerOptions: CompilerOptions; // Used as a source for typing inference
unresolvedImports: ReadonlyArray<string>; // List of unresolved module ids from imports
containsCommonJsRequire: boolean; // True if there is a `require()` call somewhere in the program
Copy link
Author

Choose a reason for hiding this comment

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

Should this be made optional? This interface is casted to in one place but I can't find anywhere we actually create it -- but it does happen to share a shape with interface DiscoverTypings in server.ts so maybe it comes from there?

@DanielRosenwasser
Copy link
Member

I just realized that there's one place this could cause problems, which is in a webpack.config.js, which apparently has its own type for require. I wonder if that's still okay?

@ghost
Copy link
Author

ghost commented Jan 3, 2018

@DanielRosenwasser What do those require calls look like? Unless there's one argument and it's a string literal, we shouldn't count it as a commonjs require.

@mhegazy
Copy link
Contributor

mhegazy commented Jan 12, 2018

I do not think this the right fix. first it is a bit of a heavy hammer for the require missing issue to include all of node. I think for this scenario we can just add something to suppress the error for require specifically in some contexts..

the other issue however is that does not handle the scenario listed in #21164, with the flashing of the error untill ATA gets to run. i think this may be different than the original problem here, but it may be related too..

let's talk about the approach we can take on Tuesday.

@mhegazy
Copy link
Contributor

mhegazy commented Apr 26, 2018

Closing in favor of the proposal in #21933

@mhegazy mhegazy closed this Apr 26, 2018
@ghost ghost deleted the commonjs_node branch May 2, 2018 22:02
@microsoft microsoft locked and limited conversation to collaborators Jul 31, 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.

2 participants