Load JavaScript modules from Node packages #7075
Conversation
|
This is awesome, I think it could be added to 1.9 or 1.8 ... very, very useful. |
|
I've not looked at the code but this looks like a very sensible/useful idea |
| const i = moduleName.lastIndexOf("./", 1); | ||
| const startsWithDotSlashOrDotDotSlash = i === 0 || (i === 1 && moduleName.charCodeAt(0) === CharacterCodes.dot); | ||
| return !startsWithDotSlashOrDotDotSlash; | ||
| return !startsWithDotSlashOrDotDotSlash.test(moduleName); |
There was a problem hiding this comment.
The idea here is that looking up the fist 2 chars is cheeper than running a regexp. is there a reason why we need to change it?
|
a few minor comments. other than that looks good to me. |
|
@mhegazy Is this still on for a 2.0 release? Is it too late for a 1.9? |
|
I could really use this getting into the 1.9 release.. pretty please! |
|
a 1.9 release would help us tremendously also |
|
+1 for getting this in the 1.9 release - would be a great help for us. |
|
It would be great if this made it to 1.9 |
|
@billti is this ready to go? can you update? |
|
I'll try and get to it later tonight or tomorrow... just got a couple of other tasks ahead of it. |
|
They say necessity is the mother of invention... by rethinking how to handle this with the new requirement that a SourceFile might be reused across programs, I think this is actually much simpler now. @mhegazy, can you take a look and let me know if you spot any flaws with this approach? I don't handle the suppression of emit if the file is located under node_modules, but that is a separate issue that is present already. I can add that and more tests if desired before checking in. |
|
Actually, trying to solve the emit problem I realized where this falls down in the general case. If module A imports B imports C, and is doesn't add C when processing B because it's too many hops, if I then later directly import B, is will already have processed the file and won't reprocess imports and see that C is now not too far and should be added. 😢 Back to the drawing board... |
|
👍 |
|
Can you also add a test for |
|
@mhegazy This should be pretty comprehensive now. It tracks if a JavaScript file was loaded from searching node_modules so it knows not to compile it (a minor tweak to this logic could also solve #6964 if desired). It also tracks if a module has some imports skipped, so that if the same module is later processed higher up the import graph it will re-processes the imports. I added a few project tests to test a) the eliding once the max depth is reached,b) increasing the depth via the config option, and c) the reprocessing of imports if a module is later imported higher up the chain. I used Project tests as with baselines its hard to add files that exist but aren't root files (as the files under node_modules need to be imported but not root files). The project tests also contain useful info about what files were input, resolved, and output. Let me know if you have any other suggestions for improvements. p.s. I had to fix a regular expression to get the Project tests working with the .js files (else it was putting a full path from my machine in the error baselines), but that did change a lot of other baselines (for the better). |
| @@ -18,7 +18,7 @@ | |||
| ==== /node_modules/bar/index.d.ts (1 errors) ==== | |||
| /// <reference types="alpha" /> | |||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | |||
| !!! message TS4090: Conflicting library definitions for 'alpha' found at 'index.d.ts' and 'index.d.ts'. Copy the correct file to the 'typings' folder to resolve this conflict. | |||
| !!! message TS4090: Conflicting library definitions for 'alpha' found at '/node_modules/bar/node_modules/alpha/index.d.ts' and '/node_modules/foo/node_modules/alpha/index.d.ts'. Copy the correct file to the 'typings' folder to resolve this conflict. | |||
There was a problem hiding this comment.
Fixing the regular expression makes messages such as this useful.
|
👍 |
|
How about this change? Note that I'm using a Map for the collection of file names as this seems quicker on large projects with lots of modules from node_modules, than using a array of file paths and scanning this every time to check for existence (and splicing when I need to remove an entry). I could use a simple string[] if that seems cleaner, but this works well currently. |
|
I would have preferred a function that takes a file path and returns a boolean. but this is fine too. |
|
can you mark this as |
|
Like so? |
|
thanks! |
|
The build is taking forever :-/ I'm sure it'll pass... but waiting as a precaution. I'll port and merge this to release-2.0 also unless you have any objections. |
|
no objections. thanks! |
I'm not looking to push this now, but opening for discussion on both a) the approach in the implementation, and b) some design questions it raises.
This change would allow for the loading of JavaScript modules from a search of top level Node packages. This could be useful in TypeScript, but is especially useful in editing JavaScript.
To avoid transitively loading 100s of JavaScript files from an NPM installed dependency graph, this sets a limit on how deep in the dependency chain to crawl (which only applies to JavaScript, not TypeScript files). For example, if
chalkdepends onhas-ansiwhich depends onansi-regex, then settingmaxNodeModuleJsDepthto 0 (the default currently) wouldn't load JavaScript modules for any of them. Setting to 1 would load only the JavaScript modules forchalk, to 2 would loadhas-ansialso, etc.. If a module exposes imports from a dependency, then a greater search depth flows up richer type information (e.g. packages such asgulpcommonly reexport members of their imported dependencies, such asvinyl-fs).In order to avoid recompiling/overwriting JavaScript files installed under
node_modules, this change also suppresses emitting any code for files found with a search of installed Node packages. This is a change in behavior (as can be seen in some baseline changes below), but what is desirable here is up for debate. It seems bad if you end up renaming a property in a dependency you installed from NPM (which will be reverted on nextnpm install, likely resulting in a mismatch of property names), yet some folks alsonpm linkacross multiple projects under development in order to treat the whole as a "solution".@vladima The approach below no longer modifies the
ResolvedModuletype (it uses the existingisExternalLibraryImportflag), but does add a member toSourceFile. Let me know if this requires managed side changes also.