Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ensure strict mode directive in all files #1005

Merged
merged 3 commits into from
May 13, 2024
Merged

Ensure strict mode directive in all files #1005

merged 3 commits into from
May 13, 2024

Conversation

Cruikshanks
Copy link
Member

DEFRA/water-abstraction-team#115

It is currently an unwritten convention (we are working on fixing that!) to add 'use strict' to the top of all our files. Why?

JavaScript's strict mode is a way to opt in to a restricted variant of JavaScript, thereby implicitly opting-out of "sloppy mode".
MSDN docs - Strict mode

When the alternate mode is named 'sloppy', would you want to use it!?

The docs do provide some further explanation of what declaring strict mode means

Strict mode makes several changes to normal JavaScript semantics:

  1. Eliminates some JavaScript silent errors by changing them to throw errors.
  2. Fixes mistakes that make it difficult for JavaScript engines to perform optimizations: strict mode code can sometimes be made to run faster than identical code that's not strict mode.
  3. Prohibits some syntax likely to be defined in future versions of ECMAScript.

For these reasons, this change updates our ESLint rules to ensure we do this.

@Cruikshanks Cruikshanks added the conventions Changes to tools that enforce our conventions label May 12, 2024
@Cruikshanks Cruikshanks self-assigned this May 12, 2024
@Cruikshanks Cruikshanks marked this pull request as ready for review May 12, 2024 21:23
Jozzey
Jozzey previously approved these changes May 13, 2024
View reviewed changes
Copy link
Contributor

@Jozzey Jozzey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Cruikshanks
Ensure strict mode directive in all files
9418616 
DEFRA/water-abstraction-team#115

It is currently an unwritten convention (we are [working on](DEFRA/water-abstraction-team#117) fixing that!) to add 'use strict' to the top of all our files. Why?

> JavaScript's strict mode is a way to opt in to a restricted variant of JavaScript, thereby implicitly opting-out of "sloppy mode".
> [MSDN docs - Strict mode](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode)

When the alternate mode is named 'sloppy', would you want to use it!?

The docs do provide some further explanation of what declaring strict mode means

> Strict mode makes several changes to normal JavaScript semantics:
>
> 1. Eliminates some JavaScript silent errors by changing them to throw errors.
> 2. Fixes mistakes that make it difficult for JavaScript engines to perform optimizations: strict mode code can sometimes be made to run faster than identical code that's not strict mode.
> 3. Prohibits some syntax likely to be defined in future versions of ECMAScript.

For these reasons, this change updates our ESLint rules to ensure we do this.
@Cruikshanks
Apply rule
17e9bf5 
To make this work we also had to tell ESLint we were not using ESM Modules as our default. It [defaults `sourceType` to 'module'](https://eslint.org/docs/latest/use/configure/language-options#specifying-javascript-options). When you use ESM modules strict mode is implied to what actually happens is ESLint shouts at us for hacving all these 'use strict' declarations!
@Cruikshanks
Set ESLint parser options to 'script'
e346840 
We may have misunderstood the ESLint docs but 'commonjs' seemed the appropriate sourceType to set.

> commonjs - CommonJS module (useful if your code uses require()). Your code has a top-level function scope and runs in non-strict mode.

Running the linting worked fine. But then we tried to run the unit tests

```
> [email protected] test
> lab --silent-skips --shuffle

Error requiring file: /home/repos/water-abstraction-system/test/controllers/bill-licences.controller.test.js
.sourceType must be "module", "script", "unambiguous", or undefined
Error: .sourceType must be "module", "script", "unambiguous", or undefined
    at validate (/home/repos/water-abstraction-system/node_modules/@babel/core/lib/config/validation/options.js:92:25)
    at loadPrivatePartialConfig (/home/repos/water-abstraction-system/node_modules/@babel/core/lib/config/partial.js:80:50)
    at loadPrivatePartialConfig.next (<anonymous>)
    at Object.<anonymous> (/home/repos/water-abstraction-system/node_modules/@babel/core/lib/config/partial.js:149:25)
    at Generator.next (<anonymous>)
    at evaluateSync (/home/repos/water-abstraction-system/node_modules/gensync/index.js:251:28)
    at Object.sync [as loadPartialConfigSync] (/home/repos/water-abstraction-system/node_modules/gensync/index.js:89:14)
    at normalizeBabelParseConfigSync (/home/repos/water-abstraction-system/node_modules/@babel/eslint-parser/lib/worker/configuration.cjs:87:24)
    at LocalClient.handleMessage (/home/repos/water-abstraction-system/node_modules/@babel/eslint-parser/lib/worker/handle-message.cjs:25:90)
    at LocalClient.<anonymous> (/home/repos/water-abstraction-system/node_modules/@babel/eslint-parser/lib/client.cjs:98:79)
    at LocalClient.maybeParse (/home/repos/water-abstraction-system/node_modules/@babel/eslint-parser/lib/client.cjs:52:47)
    at parse (/home/repos/water-abstraction-system/node_modules/@babel/eslint-parser/lib/parse.cjs:28:14)
    at Object.parse (/home/repos/water-abstraction-system/node_modules/@babel/eslint-parser/lib/index.cjs:19:10)
    at internals.instrument (/home/repos/water-abstraction-system/node_modules/@hapi/lab/lib/modules/coverage.js:327:31)
    at require.extensions.<computed> [as .js] (/home/repos/water-abstraction-system/node_modules/@hapi/lab/lib/modules/coverage.js:117:59)
    at Module.load (node:internal/modules/cjs/loader:1206:32)
    at Module._load (node:internal/modules/cjs/loader:1022:12)
    at Module.require (node:internal/modules/cjs/loader:1231:19)
    at require (node:internal/modules/helpers:179:18)
    at Object.<anonymous> (/home/repos/water-abstraction-system/test/controllers/bill-licences.controller.test.js:12:34)
    at Module._compile (node:internal/modules/cjs/loader:1369:14)
    at require.extensions.<computed> [as .js] (/home/repos/water-abstraction-system/node_modules/@hapi/lab/lib/modules/coverage.js:123:28)
    at Module.load (node:internal/modules/cjs/loader:1206:32)
    at Module._load (node:internal/modules/cjs/loader:1022:12)
    at Module.require (node:internal/modules/cjs/loader:1231:19)
    at require (node:internal/modules/helpers:179:18)
    at internals.tryToResolveCJS (/home/repos/water-abstraction-system/node_modules/mo-walk/lib/index.js:144:17)
    at exports.tryToResolve (/home/repos/water-abstraction-system/node_modules/mo-walk/lib/index.js:106:22)
    at internals.traverse (/home/repos/water-abstraction-system/node_modules/@hapi/lab/lib/cli.js:152:39)
    at async exports.run (/home/repos/water-abstraction-system/node_modules/@hapi/lab/lib/cli.js:61:21)
    at async main (/home/repos/water-abstraction-system/node_modules/@hapi/lab/bin/lab:56:22)
```

We suspect this is an issue with Hapi Lab and its dependencies (boy we love JavaScript!) Setting the sourceType to 'script' makes the tests happy and we ESLint is still doing what we want it to.
@Cruikshanks Cruikshanks force-pushed the require-strict-mode-directives branch from 2f4bae8 to e346840 Compare May 13, 2024 11:43
@Cruikshanks Cruikshanks merged commit 8c8964c into main May 13, 2024
6 checks passed
@Cruikshanks Cruikshanks deleted the require-strict-mode-directives branch May 13, 2024 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonathangoulding jonathangoulding jonathangoulding left review comments

@Jozzey Jozzey Jozzey left review comments

@robertparkinson robertparkinson robertparkinson left review comments

@Demwunz Demwunz Awaiting requested review from Demwunz

@Beckyrose200 Beckyrose200 Awaiting requested review from Beckyrose200

@rvsiyad rvsiyad Awaiting requested review from rvsiyad

Assignees

@Cruikshanks Cruikshanks

Labels
conventions Changes to tools that enforce our conventions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Cruikshanks @robertparkinson @Jozzey @jonathangoulding