Skip to content

Conversation

@billti
Copy link
Member

@billti billti commented Feb 11, 2016

This is to address #6807 and make CommonJS the default module type if not specified. Note that this changed a lot of baselines - and mostly for the better. (We had a ton of tests that were valid except for the must specify a module type error. So now these have .types and .symbol baselines, instead of just .error files).

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.

Note: Two tests are still failing in the LSHost tests. I'm not sure exactly what these are testing of how to fix them (tried a couple of ways unsuccessfully). @vladima do you know what's going on here? The two failing tests are:

2 failing
  1) Caching in LSHost works using legacy resolution logic:
     Error: NYI
      at Object.directoryExists (built/local/run.js:67284:23)
  2) Caching in LSHost loads missing files from disk:
     Error: NYI
      at Object.directoryExists (built/local/run.js:67284:23)

else {
// by default exclude node_modules, and any specificied output directory
exclude = ["node_modules"]
let outDir = json["compilerOptions"] && json["compilerOptions"]["outDir"];
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be const outDir?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes and linter will complaint about it

"category": "Error",
"code": 1147
},
"Cannot compile modules unless the '--module' flag is provided. Consider setting the 'module' compiler option in a 'tsconfig.json' file.": {
Copy link
Contributor

Choose a reason for hiding this comment

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

We have added this message in the past to catch iadvertant use of export/import in the top level of a script file. ppl used to do this by mistak, and spend hours to understand what is wrong with thier program.

@zhengbli and I talked about this, and the conclusion was to leave a valid module target --m none which will trigger this error. this way users who are sure they never need a module loader can set it and get the nice error instead of having to go crazy figuring out what it means.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well that's a pain :-/ ModuleKind.None is 0, so I'll need to change all the existing checks that just looks for a falsy value as meaning "not set". Will look into it...

@mhegazy
Copy link
Contributor

mhegazy commented Feb 11, 2016

Also note, we need to make a similar change on the project property pages in VS (2013 and 2015)

@billti
Copy link
Member Author

billti commented Feb 11, 2016

A few changes:

  • Don't default to CommonJS if the target is ES6.
  • Allow the user to explicitly set module to none.
  • Distinguish between none (numeric 0) and not specified (undefined) in the code.
  • When crawling for files to include, skip over minified files (*.min.js).

@mhegazy
Copy link
Contributor

mhegazy commented Feb 11, 2016

👍

1 similar comment
@RyanCavanaugh
Copy link
Member

👍

billti added a commit that referenced this pull request Feb 11, 2016
Make CommonJS the default (and some default "exclude" values).
@billti billti merged commit 1173473 into release-1.8 Feb 11, 2016
@billti billti deleted the CommonJSDefault branch February 11, 2016 21:33
@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.

7 participants