From b1516da92dfc034f00573bd776f59133bcf6e046 Mon Sep 17 00:00:00 2001 From: rosen-vladimirov Date: Wed, 13 Mar 2019 16:54:24 +0200 Subject: [PATCH 1/2] 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/2] 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": {