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 — Port #7486 to release 1.8 by zhengbli · Pull Request #7513 · microsoft/TypeScript

Conversation

@zhengbli

@zhengbli Zhengbo Li (zhengbli) commented Mar 15, 2016

Copy link
Copy Markdown

No description provided.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
zhengbli added 5 commits March 14, 2016 21:13
zhengbli
Add upper limit for the program size, fix readDirectory for the symli…
3b55558 
…nk files
zhengbli
Add comments
708fb97
zhengbli
CR feedback / Change upper limit / Add disableSizeLimit compiler option
35b972e 
# Conflicts:
#	src/compiler/commandLineParser.ts
#	src/compiler/diagnosticMessages.json
zhengbli
online and offline CR feedback
4aba58c
zhengbli
Don't count current opened client file if it's TS file
6ed14c2
@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Mar 15, 2016
zhengbli
Update LKG
cfa83e9
billti
billti reviewed Mar 15, 2016
View reviewed changes
Comment thread src/compiler/program.ts Outdated
if (rootLevelDirectory[rootLevelDirectory.length - 1] !== directorySeparator) {
rootLevelDirectory += directorySeparator;
}
programDiagnostics.add(createCompilerDiagnostic(Diagnostics.Too_many_JavaScript_files_in_the_project_Consider_specifying_the_exclude_setting_in_project_configuration_to_limit_included_source_folders_The_likely_folder_to_exclude_is_0_To_disable_the_project_size_limit_set_the_disableSizeLimit_compiler_option_to_true, rootLevelDirectory));

@billti Bill Ticehurst (billti) Mar 15, 2016

Copy link
Copy Markdown
Member

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.

So you are actually reporting this on the next file after the file that pushed over the size limit. So if the compiler was loading a massive file "./tmp/bundle.js" that was 20MB, followed by "./src/app.js" that was 1MB, you'd report on "./src". You're also reporting before you call host.getSourceFile, so the file/folder reported as being too large may not even exit.

Any reason you are not reporting and setting to -1 after you call host.getSourceFile and add the length to programSizeForNonTsFiles below?

Sorry, something went wrong.

Uh oh!

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

@zhengbli Zhengbo Li (zhengbli) Mar 15, 2016

Copy link
Copy Markdown
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.

I agree it makes more sense to report error right after the file exceeding the limit. Updating

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
zhengbli
CR feedback and Update LKG
5e74ebd
@billti

Bill Ticehurst (billti) commented Mar 15, 2016

Copy link
Copy Markdown
Member

I made a comment in email about ensuring we don't flood with file not found errors once we start skipping file loading. If you've verified that, as well as done some other manual testing of good/bad scenarios to ensure it all behaves as expected, then looks good to me! 👍 Thanks!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
zhengbli added 2 commits March 14, 2016 23:34
zhengbli
Avoid false "not found" errors after stopping loading non-Ts files
4d0f488
zhengbli
Merge branch 'release-1.8' of https://github.com/Microsoft/TypeScript …
f13a9c7 
…into projectSizeLimitFor18-2
@zhengbli
Zhengbo Li (zhengbli) force-pushed the projectSizeLimitFor18-2 branch from 90763f9 to f13a9c7 Compare March 15, 2016 09:28
billti
billti reviewed Mar 15, 2016
View reviewed changes
Comment thread src/compiler/program.ts Outdated
else if (!findSourceFile(fileName, toPath(fileName, currentDirectory, getCanonicalFileName), isDefaultLib, refFile, refPos, refEnd)) {
diagnostic = Diagnostics.File_0_not_found;
diagnosticArgument = [fileName];
if (hasTypeScriptFileExtension(fileName) || options.disableSizeLimit || programSizeForNonTsFiles !== -1) {

@billti Bill Ticehurst (billti) Mar 15, 2016

Copy link
Copy Markdown
Member

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 magic -1 value is being used in a few places now. Can you make this a const with a meaningful name and use that to make the code more readable (e.g. const sizeLimitExceeded = -1). It would also help readability if this was a simple helper function (e.g. function sizeLimitExceed() { return !options.disableLimit && programSizeForNonTsFiles === sizeLimitExceeded;}) used in these conditions.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
zhengbli added 3 commits March 15, 2016 10:10
zhengbli
Add exceedProgramSizeLimit function
7547f2f
zhengbli
Update LKG
23184f5
zhengbli
Remove LKG from code review
760c163
mhegazy
mhegazy reviewed Mar 15, 2016
View reviewed changes
Comment thread src/compiler/program.ts Outdated
diagnostic = Diagnostics.File_0_not_found;
fileName += ".ts";
diagnosticArgument = [fileName];
if (!exceedProgramSizeLimit()) {

@mhegazy Mohamed Hegazy (mhegazy) Mar 15, 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.

i would add it below. somethig like

if (diagnostic && !exceedProgramSizeLimit())

Sorry, something went wrong.

Uh oh!

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

Mohamed Hegazy (mhegazy) commented Mar 15, 2016

Copy link
Copy Markdown
Contributor

👍
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
zhengbli
Address PR
b0fc9e0
@billti

Bill Ticehurst (billti) commented Mar 15, 2016

Copy link
Copy Markdown
Member

👍 Thanks!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Zhengbo Li (zhengbli) pushed a commit that referenced this pull request Mar 15, 2016
Zhengbo Li
Merge pull request #7513 from zhengbli/projectSizeLimitFor18-2
b23503c 
Port #7486 to release 1.8
@zhengbli
Zhengbo Li (zhengbli) merged commit b23503c into microsoft:release-1.8 Mar 15, 2016
@zhengbli
Zhengbo Li (zhengbli) deleted the projectSizeLimitFor18-2 branch March 15, 2016 23:03
@zhengbli Zhengbo Li (zhengbli) mentioned this pull request Mar 15, 2016
tsserver crashes with large projects #7444
Closed
@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.

Uh oh!

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

4 participants

@zhengbli @billti @mhegazy @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.