JS: support the "type" field on package.json files while extracting
#4108
Conversation
| if (cache.containsKey(folder)) { | ||
| return cache.get(folder); | ||
| } | ||
| for (final File file : folder.listFiles()) { |
There was a problem hiding this comment.
Why iterate over the contents to look for a specific file?
I'd suggest just doing new File(folder, "package.json") and check that it exists and is a file.
| } | ||
|
|
||
| // cache for `getPackageType`. | ||
| private static final Map<File, String> cache = new HashMap<>(); |
There was a problem hiding this comment.
As far as I can tell, we create one ScriptExtractor per file, so this cache isn't helping when it sits here.
Also keep in mind that we extract multiple files in parallel, so when moving it to a shared place, it needs to be thread-safe.
ExtractorState should be the right place to put this cache. It's the place to share data between individual file extractions. I think we can just expose it as a ConcurrentHashMap like we do with snippets (and member to update ExtractorState#reset).
There was a problem hiding this comment.
I had to use Optional<String> because ConcurrentHashMap does not do null values. But it works great now.
| private boolean isAlwaysModule(String extension) { | ||
| return extension.equals(".mjs") || extension.equals(".es6") || extension.equals(".es"); | ||
| /** True if the file specified by `locationManager` should always be treated as a module. */ | ||
| private boolean isAlwaysModule(LocationManager locationManager, String packageType) { |
There was a problem hiding this comment.
Why pass in the LocationManager instead of extension? Seems like an unrelated change.
There was a problem hiding this comment.
At one point I computed the "type" inside isAlwaysModule, and that required the LocationManager.
I've changed it back now.
| } | ||
| try { | ||
| BufferedReader reader = new BufferedReader(new FileReader(file)); | ||
| String result = new Gson().fromJson(reader, PackageJSON.class).type; |
There was a problem hiding this comment.
Could you add a test where type is present but is not a string or null (e.g a number). It seems like this will crash the extractor.
Also catch JsonSyntaxException below in case the file has a syntax error.
There was a problem hiding this comment.
JsonSyntaxException seems to also catch the case where type is not a string.
A followup on the
.cjsfile support.Adds support for the "type" field from the
package.jsonfile in the extractor.The "type" field determines which module-type (CommonJS/ES2015) a
.jsfile should be treated as.We need this information in the extractor for e.g. determining strict-mode.
So that is why I put it into the extractor, even though it might have been easier to implement on the
.qlside.I've added a
folder->typecache that should hopefully eliminate any potential performance impact.My previous
.cjssupport PR had a bug (missing.withPlatform(Platform.NODE)), which is why the.trapfile has been updated by this PR.