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

Salsa - should not attempt to digest minified .js files #6675

Closed
egamma opened this issue Jan 27, 2016 · 11 comments
Closed

Salsa - should not attempt to digest minified .js files #6675

egamma opened this issue Jan 27, 2016 · 11 comments
Assignees
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue

Comments

@egamma
Copy link
Member

egamma commented Jan 27, 2016

Having a minified .js file in the project is almost always a hint that a folder that should be excluded in the jsconfig.json is not excluded.

@zhengbli
Copy link
Contributor

Maybe we should have a flag like ignoreMinifiedJsFiles for jsconfig.json, and by default set that to true.

@zhengbli zhengbli self-assigned this Jan 27, 2016
@zhengbli zhengbli added this to the Salsa 0.9 milestone Jan 27, 2016
@zhengbli zhengbli added the Salsa label Jan 28, 2016
@billti
Copy link
Member

billti commented Jan 28, 2016

@zhengbli Let's not add any more flags to the compiler unless we need to - there's plenty already.

First question is how to detect if a file is minified. One is very consistent and clear - the file ends in .min.js, the other is more a heuristic - such as the ratio of newlines to characters.

Personally I'd prefer not to go via heuristics where possible. Is detecting based on filename sufficient (i.e. how common is it to have minified JavaScript files that don't end in .min.js)?

@zhengbli
Copy link
Contributor

On a second thought, this with the previous suggestion that node_modules folder should be excluded by default, might potentially contribute to some obscure behavior that the user can't understand if the settings were put in the config file. For example, for a mixed project with both js files and ts files, tsconfig.json with allowJs: true might compile to different results from jsconfig.json. Therefore even though some heuristics are true most of the time, it might be better to expose them somehow so that it is easy to identify.

@mhegazy mhegazy added the Bug A bug in TypeScript label Feb 2, 2016
@mhegazy mhegazy modified the milestones: Salsa 0.9, TypeScript 1.8.2 Feb 2, 2016
@billti
Copy link
Member

billti commented Feb 2, 2016

Per further discussion, lets just go with ignoring files with ".min." in the name, and we can consider other heuristics if this is not sufficient.

Some examples of possible heuristics and where they might give incorrect results:

  • No line breaks: included license comments or preserved multi-line strings may break this.
  • Average line length: Small modules with a few very long lines would break this (e.g. (x == 10 || x == 100 || ...) type patterns in char unicode category checking).
  • Minimal use of space chars: Embedded string literals (e.g. error messages) could break this.

Perhaps if we're running it through the parser anyway we could count newlines and spaces outside of comments and strings? A more involved change, but maybe an option if the naive name check isn't sufficient.

@mhegazy
Copy link
Contributor

mhegazy commented Feb 2, 2016

who will be doing the filtering? the compiler or the server host?

@zhengbli
Copy link
Contributor

zhengbli commented Feb 3, 2016

It is probably better to do the filtering in the language service. Otherwise the project is different when editing in the editor and compiling from tsc, which might have side effects.

@mhegazy
Copy link
Contributor

mhegazy commented Feb 3, 2016

fair.

@egamma
Copy link
Member Author

egamma commented Feb 3, 2016

@billti starting with the ".min.js" pattern makes sense. It doesn't cover webpack archives that look like this static/dist/main-10964a0d0cd7d21c2c2e.js one hint here for a minified file is that there is a corresponding sourcemap file: static/dist/main-10964a0d0cd7d21c2c2e.js.map (this is from the project mentioned in this comment).

I've just tested a project with webpacks and the good news is that Salsa can digest a 1.5MB webpack file, which has caused a crash with our existing JS language service.

@billti billti assigned billti and unassigned zhengbli Feb 3, 2016
@billti
Copy link
Member

billti commented Feb 11, 2016

Fixed in #7016.

@egamma
Copy link
Member Author

egamma commented Feb 12, 2016

@billti Great thanks!

We will update our docs to address.

This change also adds a default node_modules and whatever the current outDir value is (if any) to the exclude array if there is no value provides in the config file.

❓ does this change also apply to the tsconfig.json?

@mhegazy
Copy link
Contributor

mhegazy commented Feb 12, 2016

yes.

@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
Bug A bug in TypeScript Fixed A PR has been merged for this issue
Projects
None yet
Development

No branches or pull requests

4 participants