From b1516da92dfc034f00573bd776f59133bcf6e046 Mon Sep 17 00:00:00 2001 From: rosen-vladimirov Date: Wed, 13 Mar 2019 16:54:24 +0200 Subject: [PATCH 1/3] fix: warnings for short imports should be shown correctly In case you have a minified content or just several statements on the same line, we may print you a warning that you have short imports, in cases where you do not have such. Fix this by splitting the content by new line and by `;` after that. This way, in case you have multiple statements on the same line, we'll separate them and should be able to check them correctly. Also, ensure `node_modules` of the application are not analyzed. NOTE: Currently in case you have a multiline string, like: ``` const lodash = require("lodash");console.log(`this is multiline string that has require "application" in it and ends somewhere else`); ``` We'll detect the `that has require "application" in it` line as short import. Add test to show that this is the intended behavior at the moment, as fixing it requires additional complex logic without much benefit. --- lib/services/doctor-service.ts | 4 +- lib/services/project-data-service.ts | 17 +++++++- test/services/doctor-service.ts | 60 +++++++++++++++++++++++----- 3 files changed, 68 insertions(+), 13 deletions(-) diff --git a/lib/services/doctor-service.ts b/lib/services/doctor-service.ts index 0e9d6bae5e..b45311af04 100644 --- a/lib/services/doctor-service.ts +++ b/lib/services/doctor-service.ts @@ -141,8 +141,10 @@ export class DoctorService implements IDoctorService { for (const file of files) { const fileContent = this.$fs.readText(file); const strippedComments = helpers.stripComments(fileContent); - const linesWithRequireStatements = strippedComments + const linesToCheck = _.flatten(strippedComments .split(/\r?\n/) + .map(line => line.split(";"))); + const linesWithRequireStatements = linesToCheck .filter(line => /\btns-core-modules\b/.exec(line) === null && (/\bimport\b/.exec(line) || /\brequire\b/.exec(line))); for (const line of linesWithRequireStatements) { diff --git a/lib/services/project-data-service.ts b/lib/services/project-data-service.ts index 65578062ee..6a4cb20bbd 100644 --- a/lib/services/project-data-service.ts +++ b/lib/services/project-data-service.ts @@ -1,7 +1,15 @@ import * as path from "path"; import { ProjectData } from "../project-data"; import { exported } from "../common/decorators"; -import { NATIVESCRIPT_PROPS_INTERNAL_DELIMITER, AssetConstants, SRC_DIR, RESOURCES_DIR, MAIN_DIR, CLI_RESOURCES_DIR_NAME, ProjectTypes } from "../constants"; +import { + NATIVESCRIPT_PROPS_INTERNAL_DELIMITER, + AssetConstants, SRC_DIR, + RESOURCES_DIR, + MAIN_DIR, + CLI_RESOURCES_DIR_NAME, + ProjectTypes, + NODE_MODULES_FOLDER_NAME +} from "../constants"; interface IProjectFileData { projectData: any; @@ -124,6 +132,7 @@ export class ProjectDataService implements IProjectDataService { supportedFileExtension = ".ts"; } + const pathToProjectNodeModules = path.join(projectDir, NODE_MODULES_FOLDER_NAME); const files = this.$fs.enumerateFilesInDirectorySync( projectData.appDirectoryPath, (filePath, fstat) => { @@ -132,6 +141,12 @@ export class ProjectDataService implements IProjectDataService { } if (fstat.isDirectory()) { + if (filePath === pathToProjectNodeModules) { + // we do not want to get the files from node_modules directory of the project. + // We'll get here only when you have nsconfig.json with appDirectoryPath set to "." + return false; + } + return true; } diff --git a/test/services/doctor-service.ts b/test/services/doctor-service.ts index 0b55e96b9f..0074c806cd 100644 --- a/test/services/doctor-service.ts +++ b/test/services/doctor-service.ts @@ -49,7 +49,7 @@ describe("doctorService", () => { filesContents: { file1: 'const application = require("application");' }, - expectedShortImports: [{ file: "file1", line: 'const application = require("application");' }] + expectedShortImports: [{ file: "file1", line: 'const application = require("application")' }] }, { filesContents: { @@ -61,7 +61,7 @@ describe("doctorService", () => { filesContents: { file1: 'const Observable = require("data/observable").Observable;' }, - expectedShortImports: [{ file: "file1", line: 'const Observable = require("data/observable").Observable;' }] + expectedShortImports: [{ file: "file1", line: 'const Observable = require("data/observable").Observable' }] }, { filesContents: { @@ -73,7 +73,7 @@ describe("doctorService", () => { filesContents: { file1: 'import * as application from "application";' }, - expectedShortImports: [{ file: "file1", line: 'import * as application from "application";' }] + expectedShortImports: [{ file: "file1", line: 'import * as application from "application"' }] }, { filesContents: { @@ -85,7 +85,7 @@ describe("doctorService", () => { filesContents: { file1: 'import { run } from "application";' }, - expectedShortImports: [{ file: "file1", line: 'import { run } from "application";' }] + expectedShortImports: [{ file: "file1", line: 'import { run } from "application"' }] }, { filesContents: { @@ -98,7 +98,7 @@ describe("doctorService", () => { filesContents: { file1: "import { run } from 'application';" }, - expectedShortImports: [{ file: "file1", line: "import { run } from 'application';" }] + expectedShortImports: [{ file: "file1", line: "import { run } from 'application'" }] }, { // Using single quotes @@ -114,8 +114,8 @@ const Observable = require("data/observable").Observable; ` }, expectedShortImports: [ - { file: "file1", line: 'const application = require("application");' }, - { file: "file1", line: 'const Observable = require("data/observable").Observable;' }, + { file: "file1", line: 'const application = require("application")' }, + { file: "file1", line: 'const Observable = require("data/observable").Observable' }, ] }, { @@ -125,7 +125,7 @@ const Observable = require("tns-core-modules/data/observable").Observable; ` }, expectedShortImports: [ - { file: "file1", line: 'const application = require("application");' }, + { file: "file1", line: 'const application = require("application")' }, ] }, { @@ -137,8 +137,8 @@ const Observable = require("tns-core-modules/data/observable").Observable; const Observable = require("data/observable").Observable;` }, expectedShortImports: [ - { file: "file1", line: 'const application = require("application");' }, - { file: "file2", line: 'const Observable = require("data/observable").Observable;' }, + { file: "file1", line: 'const application = require("application")' }, + { file: "file2", line: 'const Observable = require("data/observable").Observable' }, ] }, { @@ -150,7 +150,45 @@ const Observable = require("tns-core-modules/data/observable").Observable; file2: `const application = require("some-name-tns-core-modules-widgets/application"); const Observable = require("tns-core-modules-widgets/data/observable").Observable;` }, - expectedShortImports: [ ] + expectedShortImports: [] + }, + { + filesContents: { + // several statements on one line + file1: 'const _ = require("lodash");console.log("application");' + }, + expectedShortImports: [] + }, + { + filesContents: { + // several statements on one line with actual short imports + file1: 'const _ = require("lodash");const application = require("application");console.log("application");', + file2: 'const _ = require("lodash");const application = require("application");const Observable = require("data/observable").Observable;' + }, + expectedShortImports: [ + { file: "file1", line: 'const application = require("application")' }, + { file: "file2", line: 'const application = require("application")' }, + { file: "file2", line: 'const Observable = require("data/observable").Observable' }, + ] + }, + { + filesContents: { + // several statements on one line withoutshort imports + file1: 'const _ = require("lodash");const application = require("tns-core-modules/application");console.log("application");', + file2: 'const _ = require("lodash");const application = require("tns-core-modules/application");const Observable = require("tns-core-modules/data/observable").Observable;' + }, + expectedShortImports: [] + }, + { + // Incorrect behavior, currently by design + // In case you have a multiline string and one of the lines matches our RegExp we'll detect it as short import + filesContents: { + file1: 'const _ = require("lodash");const application = require("application");console.log("application");console.log(`this is line\nyou should import some long words here "application" module and other words here`)', + }, + expectedShortImports: [ + { file: "file1", line: 'const application = require("application")' }, + { file: "file1", line: 'you should import some long words here "application" module and other words here`)' }, + ] }, ]; From f6c1ea872e38b1f0a4a5e1c820c4f6910244cd71 Mon Sep 17 00:00:00 2001 From: fatme Date: Fri, 15 Mar 2019 12:21:41 +0200 Subject: [PATCH 2/3] chore: update version --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 4f02a9c723..2245b512b3 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "nativescript", "preferGlobal": true, - "version": "5.2.3", + "version": "5.2.4", "author": "Telerik ", "description": "Command-line interface for building NativeScript projects", "bin": { From 3404a909ff64c46be2b421a1717946d00bcb4806 Mon Sep 17 00:00:00 2001 From: TsvetanMilanov Date: Mon, 18 Mar 2019 19:24:03 +0200 Subject: [PATCH 3/3] Don't change the options in httpRequestCore Do not change the options object in the httpRequestCore method of the $httpClient because if we do, we can't retry correctly the http requests --- lib/common/http-client.ts | 51 ++++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 25 deletions(-) diff --git a/lib/common/http-client.ts b/lib/common/http-client.ts index e9ce382367..bcaf9f3344 100644 --- a/lib/common/http-client.ts +++ b/lib/common/http-client.ts @@ -56,30 +56,30 @@ export class HttpClient implements Server.IHttpClient { }; } - const unmodifiedOptions = _.clone(options); + const clonedOptions = _.cloneDeep(options); - if (options.url) { - const urlParts = url.parse(options.url); + if (clonedOptions.url) { + const urlParts = url.parse(clonedOptions.url); if (urlParts.protocol) { - options.proto = urlParts.protocol.slice(0, -1); + clonedOptions.proto = urlParts.protocol.slice(0, -1); } - options.host = urlParts.hostname; - options.port = urlParts.port; - options.path = urlParts.path; + clonedOptions.host = urlParts.hostname; + clonedOptions.port = urlParts.port; + clonedOptions.path = urlParts.path; } - const requestProto = options.proto || "http"; - const body = options.body; - delete options.body; - let pipeTo = options.pipeTo; - delete options.pipeTo; + const requestProto = clonedOptions.proto || "http"; + const body = clonedOptions.body; + delete clonedOptions.body; + let pipeTo = options.pipeTo; // Use the real stream because the _.cloneDeep can't clone the internal state of a stream. + delete clonedOptions.pipeTo; const cliProxySettings = await this.$proxyService.getCache(); - options.headers = options.headers || {}; - const headers = options.headers; + clonedOptions.headers = clonedOptions.headers || {}; + const headers = clonedOptions.headers; - await this.useProxySettings(proxySettings, cliProxySettings, options, headers, requestProto); + await this.useProxySettings(proxySettings, cliProxySettings, clonedOptions, headers, requestProto); if (!headers.Accept || headers.Accept.indexOf("application/json") < 0) { if (headers.Accept) { @@ -115,21 +115,21 @@ export class HttpClient implements Server.IHttpClient { isResolved: () => false }; - if (options.timeout) { + clonedOptions.url = clonedOptions.url || `${clonedOptions.proto}://${clonedOptions.host}${clonedOptions.path}`; + if (clonedOptions.timeout) { timerId = setTimeout(() => { - this.setResponseResult(promiseActions, cleanupRequestData, { err: new Error(`Request to ${unmodifiedOptions.url} timed out.`) }); - }, options.timeout); + this.setResponseResult(promiseActions, cleanupRequestData, { err: new Error(`Request to ${clonedOptions.url} timed out.`) }); + }, clonedOptions.timeout); cleanupRequestData.timers.push(timerId); - delete options.timeout; + delete clonedOptions.timeout; } - options.url = options.url || `${options.proto}://${options.host}${options.path}`; - options.encoding = null; - options.followAllRedirects = true; + clonedOptions.encoding = null; + clonedOptions.followAllRedirects = true; - this.$logger.trace("httpRequest: %s", util.inspect(options)); - const requestObj = request(options); + this.$logger.trace("httpRequest: %s", util.inspect(clonedOptions)); + const requestObj = request(clonedOptions); cleanupRequestData.req = requestObj; requestObj @@ -151,7 +151,7 @@ export class HttpClient implements Server.IHttpClient { stuckRequestTimerId = setTimeout(() => { this.setResponseResult(promiseActions, cleanupRequestData, { err: new Error(HttpClient.STUCK_REQUEST_ERROR_MESSAGE) }); - }, options.timeout || HttpClient.STUCK_REQUEST_TIMEOUT); + }, clonedOptions.timeout || HttpClient.STUCK_REQUEST_TIMEOUT); cleanupRequestData.timers.push(stuckRequestTimerId); @@ -230,6 +230,7 @@ export class HttpClient implements Server.IHttpClient { const response = await result; if (helpers.isResponseRedirect(response.response)) { + const unmodifiedOptions = _.cloneDeep(options); if (response.response.statusCode === HttpStatusCodes.SEE_OTHER) { unmodifiedOptions.method = "GET"; }