BI-9132: set engine and npm restrictions; update packages; fix versio… #98

dgolubevs · 2022-11-02T12:20:39Z

JIRA link

https://companieshouse.atlassian.net/browse/BI-9132

Change description

add nmprc, nvmrc
update package versions
make changes in files related to migration to 16 version

…n issues;

SteveHicksCH

A couple of comments which are probably more for my education than anything else

tsconfig.json

scss/_variables.scss

mouhajer-ch

Could you please make sure that the description and Jira link are set correctly.

mouhajer-ch · 2022-11-11T07:13:41Z

.eslintrc

+    "extends": [
+        "eslint:recommended",
+        "plugin:@typescript-eslint/recommended",
+        "standard"


Has to be agreed across other services. I use these recommended

"eslint:recommended", "plugin:@typescript-eslint/eslint-recommended", "plugin:@typescript-eslint/recommended"

and please we do not need trailing commas (RIP IE8)

trailing commas need, not because IE8
https://time2hack.com/are-you-using-trailing-commas-in-your-javascript/
https://medium.com/@nikgraf/why-you-should-enforce-dangling-commas-for-multiline-statements-d034c98e36f8

mouhajer-ch · 2022-11-11T07:27:07Z

.eslintrc

+        ],
+        "object-curly-newline": [
+            "error"
+        ],


Played a bit with it. This is one possible config

{ "parserOptions": { "ecmaVersion": 8, "sourceType": "module", "ecmaFeatures": {} }, "extends": [ "eslint:recommended", "plugin:@typescript-eslint/eslint-recommended", "plugin:@typescript-eslint/recommended" ], "parser": "@typescript-eslint/parser", "plugins": [ "@typescript-eslint", "chai-friendly" ], "overrides": [ { "files": ["config.ts"], "rules": { "no-process-env": "off" } }, { "files": ["*.test.ts", "*.spec.ts"], "rules": { "no-process-env": "off", "no-unused-expressions": "off", "chai-friendly/no-unused-expressions": 2, "no-magic-numbers": "off" } } ], "rules": { "no-console": "warn", "no-eval": "error", "eol-last": ["error", "always"], "object-curly-spacing": [ "error", "always" ], "indent": ["error", 4], "padded-blocks": "off", "quotes": ["error", "double"], "semi": ["error", "always"], "space-before-function-paren": ["error", { "anonymous": "always", "named": "never" } ], "operator-linebreak": ["error", "after"], "no-undef": "off", "no-extra-semi": 2, "no-process-env": 2, "no-unused-vars": "off", "@typescript-eslint/no-unused-vars": "error", "@typescript-eslint/no-explicit-any": "off", "@typescript-eslint/camelcase": "off", "@typescript-eslint/explicit-function-return-type": "off", "@typescript-eslint/no-magic-numbers": "off" }, "env": { "node": true, "commonjs": true, "mocha": true, "es6": true } }

mouhajer-ch · 2022-11-11T07:28:23Z

Dockerfile

@@ -1,10 +1,11 @@
-FROM 169942020521.dkr.ecr.eu-west-1.amazonaws.com/base/node:14-alpine-builder
-FROM 169942020521.dkr.ecr.eu-west-1.amazonaws.com/base/node:14-alpine-runtime
+FROM 169942020521.dkr.ecr.eu-west-1.amazonaws.com/base/node:16-alpine-builder


Great work, finally we can move to the 16

mouhajer-ch · 2022-11-11T07:39:21Z

scss/_variables.scss

@@ -1,3 +1,3 @@
-$govuk-assets-path: "/restricted-word/public/";
+$govuk-assets-path: "/admin/restricted-word/public/";


We should remove sass files from the service. Compile it once and add it on our CDN. I have done it in other service, will improve maintainability, performance and reduces dependencies issue (gulp, gulp-sass, node-sass ...). Check here

sass files compiled to css
this change required because by this path browser is looking font files

gulp can be completele removed from project.
all what doing gulp can be done in npm commends

"build:sass": "node-sass --importer node_modules/node-sass-package-importer/dist/cli.js -o dist/stylesheets ./scss/application.scss", "build:views": "cp -r views dist/", "build:govuk": "cp -v -r node_modules/govuk-frontend/govuk/assets/**/* ./dist", "build": "npm run tsc && npm run build:views && npm run build:sass && npm run build:govuk", "build:prod": "npm run build && webpack --mode production",

^^^ Good points, but separate PR? Could we debate this outside of this PR, agree, and raise a ticket?

this change required to give ability to browser find font and images

gulp changes should be in other story

mouhajer-ch · 2022-11-11T07:42:00Z

src/app.ts

 app.use(sessionMiddleware);
-app.use(helmet());
+app.use(helmet({
+    contentSecurityPolicy: false,


Why you want to disable CSP? Check here and here for help/examples

src/controllers/RestrictedWordController.ts

mouhajer-ch · 2022-11-11T07:57:14Z

tsconfig.json

        "experimentalDecorators": true,
        "strictNullChecks": true,
        "sourceMap": true,
+        "noImplicitAny": false,


Good one, "noImplicitAny": false should be set in all other project.

Could you please make sure lint is called by our sonar, it does not seem called, don't you think? we have got many config example in other repo

We will add Sonar in a separate PR

Shouldn't noImplicitAny be true instead? Setting it to false turns off the feature and might mask errors in type declarations

mouhajer-ch · 2022-11-11T07:58:13Z

webpack.config.js

 const path = require("path");

 module.exports = {
+    mode: "development",


I do not understand what client/fancyFramework.js is used for. Is it for production? can be moved to our CDN and reduce our dependency on web pack?

I don't understand also

mouhajer-ch · 2022-11-11T08:04:15Z

Sonar does not seem correctly configured in this service, we need to check the quality of our PRs including this one.

dgolubevs · 2022-12-02T16:30:26Z

done changes please review

SteveHicksCH

Looks fine by me

ch-tstrickland · 2022-12-06T14:52:20Z

Sonar does not seem correctly configured in this service, we need to check the quality of our PRs including this one.

We will add Sonar in a separate PR

This is needed to specify the version of Node used when run.

npm version 8 is not yet supported in the build system.

BI-9132: set engine and npm restrictions; update packages; fix versio…

88e8a8a

…n issues;

SteveHicksCH approved these changes Nov 2, 2022

View reviewed changes

tsconfig.json Outdated Show resolved Hide resolved

scss/_variables.scss Show resolved Hide resolved

dgolubevs force-pushed the feature/BI-9132-upgrade-to-node16 branch from 88e8a8a to c4b2aec Compare November 3, 2022 13:29

BI-9132: strictNullChecks restore

f00cdff

dgolubevs force-pushed the feature/BI-9132-upgrade-to-node16 branch from c4b2aec to f00cdff Compare November 3, 2022 13:38

ch-tstrickland requested a review from mouhajer-ch November 10, 2022 14:58

mouhajer-ch reviewed Nov 11, 2022

View reviewed changes

BI-9132: add import aliasing; use ts-mocha;

508913a

SteveHicksCH approved these changes Dec 2, 2022

View reviewed changes

dgolubevs requested review from SVarallo, markpit and mwestacott December 6, 2022 11:53

ch-tstrickland added 3 commits December 16, 2022 12:22

Add NODE_ENV to start.sh

be26db1

This is needed to specify the version of Node used when run.

Fix npm version to 7

dd625e8

npm version 8 is not yet supported in the build system.

Trigger build

203bf9f

ch-tstrickland mentioned this pull request Dec 19, 2022

Feature/bi 9132 upgrade to node16 sonar #104

Open

		@@ -1,3 +1,3 @@
		$govuk-assets-path: "/restricted-word/public/";
		$govuk-assets-path: "/admin/restricted-word/public/";

BI-9132: set engine and npm restrictions; update packages; fix versio… #98

Are you sure you want to change the base?

BI-9132: set engine and npm restrictions; update packages; fix versio… #98

Uh oh!

Conversation

dgolubevs commented Nov 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

JIRA link

Change description

Uh oh!

SteveHicksCH left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mouhajer-ch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mouhajer-ch Nov 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mouhajer-ch Nov 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mouhajer-ch Nov 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mouhajer-ch commented Nov 11, 2022

Uh oh!

dgolubevs commented Dec 2, 2022

Uh oh!

SteveHicksCH left a comment

Choose a reason for hiding this comment

Uh oh!

ch-tstrickland commented Dec 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dgolubevs commented Nov 2, 2022 •

edited

Loading

mouhajer-ch Nov 11, 2022 •

edited

Loading

mouhajer-ch Nov 11, 2022 •

edited

Loading

mouhajer-ch Nov 11, 2022 •

edited

Loading