Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork40
chore: update deps#1141
base:master
Are you sure you want to change the base?
chore: update deps#1141
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| const config = webpackConfig(input); | ||
| expect(config.entry["tns_modules/tns-core-modules/inspector_modules"]).toEqual("inspector_modules"); | ||
| }); | ||
| // if (platform === "ios") { |
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.
Should these commented / removed?
| @@ -4,7 +4,7 @@ const { join, relative, isAbsolute } = require("path"); | |||
| const os = require("os"); | |||
| const { mkdir } = require("shelljs"); | |||
| const { get } = require("request"); | |||
| //const { get } = require("request"); | |||
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.
More commented code here?
| "source-map-support": "^0.5.13", | ||
| "tns-core-modules": "next", | ||
| "typescript": "~3.5.3" | ||
| "@nativescript/core": "^6.5.8", |
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.
nit: the deps / dev deps should be alphabetised as is usually done automagically by npm, you can get npm to do it for you if you just install a dep at the version already in here
| "name": "nativescript-dev-webpack", | ||
| "version": "1.5.0", | ||
| "name": "@nativescript/webpack", | ||
| "version": "2.0.0-alpha.3", |
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.
There's a really strange divergence between the published version and the released version in this repo, a changelog or proper release notes on the tagged releases would help clear this up a lot
| @@ -28,12 +28,12 @@ | |||
| "chai": "4.2.0", | |||
| "mochawesome": "~3.1.2", | |||
| "nativescript-dev-appium": "next", | |||
| "nativescript-dev-webpack": "next", | |||
| "@nativescript/webpack": "next", | |||
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.
nit: order
| @@ -28,11 +28,11 @@ | |||
| "mocha": "~5.2.0", | |||
| "mochawesome": "~3.1.2", | |||
| "nativescript-dev-appium": "next", | |||
| "nativescript-dev-webpack": "next", | |||
| "@nativescript/webpack": "next", | |||
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.
nit: order
| @@ -42,12 +42,12 @@ | |||
| "chai": "4.2.0", | |||
| "mochawesome": "~3.1.2", | |||
| "nativescript-dev-appium": "next", | |||
| "nativescript-dev-webpack": "next", | |||
| "@nativescript/webpack": "next", | |||
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.
nit: order
| registerModules = `(?<!${escapeRegExp(ignoredFiles[key])})` + registerModules; | ||
| } | ||
| registerModules = new RegExp(registerModules); | ||
| registerModules =<any>new RegExp(registerModules); |
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.
Opt foras T for type casting.
This any casting seems dangerous and seemingly dances around the typings issues that exist here, while functionally identical to before, is it safe? Looks more like the upstream typings are not ideal for this
Personally i'd opt for something more typesafe given the typings forignoredFiles:
letregisterModulesSafe:RegExp;if(!registerModules){registerModules=defaultMatch;// I assume you don't want to use a booleanif(Array.isArray(ignoredFiles)){for(constignoredFileofignoredFiles){registerModules+=`(?<!${escapeRegExp(ignoredFile)})`;}}elseif(typeofignoredFiles==="string"){registerModules+=`(?<!${escapeRegExp(ignoredFiles)})`;}registerModulesSafe=newRegExp(registerModules)asany;}
Then you can more safely load this expression further down:
}elseif(registerModules||registerModulesSafe){...constcontext=require.context("~/",true,${registerModules||registerModulesSafe});...}
| @@ -26,7 +26,7 @@ const loader: loader.Loader = function (content: string, map) { | |||
| } | |||
| }); | |||
| const str = JSON.stringify(ast, (k, v) => k === "position" ? undefined : v); | |||
| this.callback(null, `${dependencies.join("\n")}module.exports = ${str};`, map); | |||
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.
Is removingmap here safe? Is this for sourcemap generation?
No description provided.