GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Don't emit any source files found under node_modules by billti · Pull Request #9421 · microsoft/TypeScript

Conversation

@billti

@billti Bill Ticehurst (billti) commented Jun 29, 2016

Copy link
Copy Markdown
Member

This fixes #6964 to suppress emit for any source files located by searching in node_modules, not just JavaScript files.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@billti
Don't emit source files found under node_modules
5f8cf1a
@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Jun 29, 2016
mhegazy
mhegazy reviewed Jun 29, 2016
View reviewed changes
Comment thread src/compiler/program.ts Outdated
getSourceFileByPath: program.getSourceFileByPath,
getSourceFiles: program.getSourceFiles,
getFilesFromNodeModules: () => jsFilesFoundSearchingNodeModules,
isSourceFileFromNodeModules: (file: SourceFile) => !!lookUp(sourceFilesFoundSearchingNodeModules, file.path),

@mhegazy Mohamed Hegazy (mhegazy) Jun 29, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

we have isExternalLibraryImport, so why not call this isSourceFileFromExternalLibrary ?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mhegazy

Mohamed Hegazy (mhegazy) commented Jun 29, 2016

Copy link
Copy Markdown
Contributor

👍
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@billti Bill Ticehurst (billti) mentioned this pull request Jun 29, 2016
Refactor module resolution logic to have configurable goal extensions #9430
Closed
Bill Ticehurst (billti) added 2 commits June 29, 2016 17:04
@billti
Dont load JavaScript if types packages are present
5a45c44
@billti
Renamed API
d8047b6
@billti

Bill Ticehurst (billti) commented Jun 30, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Added fix for #9427, which was caused by related changes for JavaScript. Also changed the API per Mohamed Hegazy (@mhegazy) 's suggestion.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@billti

Bill Ticehurst (billti) commented Jun 30, 2016

Copy link
Copy Markdown
Member Author

Hmmm, must have missed a test file in a commit... all passed on my machine. Will take a look shortly.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@billti
Merge remote-tracking branch 'upstream/master' into dontEmitNodeModules
573bfec
billti
billti reviewed Jun 30, 2016
View reviewed changes
Comment thread ...s/reference/project/nodeModulesMaxDepthIncreased/amd/nodeModulesMaxDepthIncreased.errors.txt Outdated
@@ -1,4 +1,5 @@
maxDepthIncreased/root.ts(4,1): error TS2322: Type 'string' is not assignable to type 'number'.
maxDepthIncreased/root.ts(7,1): error TS2322: Type 'string' is not assignable to type 'number'.
maxDepthIncreased/root.ts(8,13): error TS2339: Property 'test' does not exist on type 'typeof "C:/src/TypeScript/tests/cases/projects/NodeModulesSearch/maxDepthIncreased/node_modules/@...'.

@billti Bill Ticehurst (billti) Jun 30, 2016

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This is causing the issue in the GitHub build... need to figure out where the full path is creeping in to the error message.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@billti
Removed one error to avoid full path issues
5e4f13f
@billti

Bill Ticehurst (billti) commented Jun 30, 2016

Copy link
Copy Markdown
Member Author

I removed that one check causing the full path to be in the error for now. The subsequent line would be in error if the wrong file was referenced anyway, so this doesn't really lose any coverage. Hopefully the tests will be green on the CI server (eventually 😩 ) after this change.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@billti Bill Ticehurst (billti) mentioned this pull request Jul 1, 2016
Port #9421 to not compile files under from node_modules #9459
Merged
@billti
Bill Ticehurst (billti) merged commit 613e2d3 into master Jul 1, 2016
@billti
Bill Ticehurst (billti) deleted the dontEmitNodeModules branch July 1, 2016 02:47
Bill Ticehurst (billti) added a commit that referenced this pull request Jul 1, 2016
@billti
Merge pull request #9459 from Microsoft/dontEmitNodeModules-2.0
f7c9a77 
Port #9421 to not compile files under from node_modules
@cartant Nicholas Jamieson (cartant) mentioned this pull request Aug 5, 2016
Refactored to use canonical file names TypeStrong/tsify#171
Merged
@microsoft 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.

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Discovering .ts files under node_modules resulting in them being compiled

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@billti @mhegazy @RyanCavanaugh @msftclas
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.