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.

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

Run prettier and lint #806

Merged
merged 5 commits into from
Mar 26, 2024
Merged

Conversation

pdcastro
Copy link
Contributor

@pdcastro pdcastro commented Mar 24, 2024

While working on another PR (not pushed yet), I found that npm run format and npm run lint would modify not only the code I was writing, but also pre-existing files in the main branch. In order to avoid unnecessary noise in my other PR, I thought I would sort the pre-existing issues first, through this PR.

While at it, @nielsfaber here’s a question: Are contributors supposed to run npm run build that calls babel? I have found that file dist/scheduler-card.js in release v3.2.12 (current latest), as well as the same file in the main branch, looks more like babel or npm run build were not executed. It looks more like it was created with just npm run rollup. Having said that, I was not able to reproduce the exact file with either command. Here’s what I get:

$ nvm install --lts
$ nvm use --lts
$ node --version
v20.11.1
$ npm --version
10.2.4
$ cd scheduler-card
$ git checkout main
$ rm -rf package-lock.json node_modules
$ npm install
$ npm run rollup
$ ls -l dist/scheduler-card.js
# The original file was about 6kB smaller
-rw-r--r--  1 paulo  staff  318436 24 Mar 22:33 dist/scheduler-card.js

$ npm run build
# eslint fails. I fix the eslint issues as per changes in this PR.

$ npm run build
> [email protected] build
> npm run lint && npm run rollup && npm run babel
> eslint src/**/*.ts --fix
> rollup -c
src/scheduler-card.ts → dist...
> babel dist/scheduler-card.js --out-file dist/scheduler-card.js

SyntaxError: dist/scheduler-card.js: Unexpected token (34:78)
  32 |      * Copyright 2017 Google LLC
  33 |      * SPDX-License-Identifier: BSD-3-Clause
> 34 |      */,le=(e,t)=>"method"===t.kind&&t.descriptor&&!("value"in t.descriptor)?{...t,finisher(i){i.createProperty(t.key,e)}}:{kind:"field",key:Symbol(),placement:"own",descriptor:{},originalKey:t.key,initializer(){"function"==typeof t.initializer&&(this[t.key]=t.initializer.call(this))},finisher(i){i.createProperty(t.key,e)}};function de(e){return(t,i)=>void 0!==i?((e,t,i)=>{t.constructor.createProperty(i,e)})(e,t,i):le(e,t)
     |                                                                               ^

# I found that the error above goes away if the babel CLI in ‘devDependencies’
# is upgraded from `"babel-cli": "^6.26.0"` to `"@babel/cli": "^7.24.1"`, however 
# `dist/scheduler-card.js` then increases in size by more than 100kB!
$ rm -rf package-lock.json node_modules
$ npm install
$ npm run build
$ ls -l dist/scheduler-card.js
-rw-r--r--  1 paulo  staff  422383 24 Mar 22:41 dist/scheduler-card.js

What is the correct procedure to generate dist/scheduler-card.js?

html`
<ha-icon icon="${icon}"></ha-icon>
`
)}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The changes to this file are a result of running npm run format. (I did not edit this file “manually”.)

@@ -41,14 +41,14 @@ export const parseVariable = (
const res =
'template' in config && isDefined(config.template)
? { ...omit(config, 'template'), ...config.template(stateObj, hass) }
: <ListVariableConfig | LevelVariableConfig | TextVariableConfig>{ ...config };
: ({ ...config } as ListVariableConfig | LevelVariableConfig | TextVariableConfig);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The changes to this file were done “manually” by me, in order to address the following npm run lint errors:

$ npm run lint
...
nielsfaber/scheduler-card/src/standard-configuration/variables.ts
  44:9   error  Use 'as ListVariableConfig | LevelVariableConfig | TextVariableConfig' instead of '<ListVariableConfig | LevelVariableConfig | TextVariableConfig>'  @typescript-eslint/consistent-type-assertions
  51:12  error  Use 'as TextVariableConfig' instead of '<TextVariableConfig>'                                                                                        @typescript-eslint/consistent-type-assertions

@@ -14,8 +14,10 @@ module.exports = {
"@typescript-eslint/camelcase": 0,
"@typescript-eslint/no-explicit-any": "off",
"@typescript-eslint/explicit-function-return-type": "off",
"@typescript-eslint/no-empty-function": "off",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This rule exception was added to suppress the following npm run lint errors:

nielsfaber/scheduler-card/src/data/websockets.ts
  48:20  error  Unexpected empty arrow function 'confirm'  @typescript-eslint/no-empty-function
  49:19  error  Unexpected empty arrow function 'cancel'   @typescript-eslint/no-empty-function

"@typescript-eslint/no-non-null-assertion": "off",
"@typescript-eslint/no-unused-vars": "off",
"@typescript-eslint/no-use-before-define": "off",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This rule exception was added to suppress the following npm run lint errors:

nielsfaber/scheduler-card/src/standard-configuration/standardActions.ts
  35:28  error  'parseAction' was used before it was defined          @typescript-eslint/no-use-before-define
  79:16  error  'parseActionVariable' was used before it was defined  @typescript-eslint/no-use-before-define

nielsfaber/scheduler-card/src/standard-configuration/standardStates.ts
  38:33  error  'getStateName' was used before it was defined  @typescript-eslint/no-use-before-define

nielsfaber/scheduler-card/src/standard-configuration/variables.ts
  47:12  error  'parseListVariable' was used before it was defined                                                                                                   @typescript-eslint/no-use-before-define
  49:12  error  'parseLevelVariable' was used before it was defined                                                                                                  @typescript-eslint/no-use-before-define

@nielsfaber
Copy link
Owner

Hello, thank you for helping out.
I would like to mention that I’m working on a new version of the card which is a complete rewrite of the code.
For that reason I think it’s best not to spend any significant effort on PRs at this point.
Depending on the work you are preparing, I could create a branch for the new version and you could use that for improvement.

@nielsfaber
Copy link
Owner

To answer your question, I only use npm start.
This creates new output in the /dist folder and updates this output when any code in /src is changed.

@pdcastro
Copy link
Contributor Author

Hello, thank you for helping out. I would like to mention that I’m working on a new version of the card which is a complete rewrite of the code. For that reason I think it’s best not to spend any significant effort on PRs at this point. Depending on the work you are preparing, I could create a branch for the new version and you could use that for improvement.

Thanks for letting me know. It was too late for PR #808 as I had already done the work. I have added a commit to this PR that proposes adding a CONTRIBUTING.md file that includes a note about this complete rewrite, hoping to warn others before they start any large pieces of work.

@pdcastro
Copy link
Contributor Author

To answer your question, I only use npm start. This creates new output in the /dist folder and updates this output when any code in /src is changed.

Got it. 👍 Here’s an interesting thing that I noticed: Deleting the Babel dependency and related packages (including rollup-plugin-babel), and removing references to babel from rollup.config.js, makes no difference to the output file (dist/scheduler-card.js). I have added a commit to this PR that removes Babel, please try it at your end. If you can confirm that removing Babel makes no difference at your end, there are advantages to its removal:

  • It fixes several security vulnerabilities reported by npm install:
    • Before removing Babel: 13 vulnerabilities (2 low, 11 high severity)
    • After removing Babel: 2vulnerabilities (0 low, 2 high severity)
  • It reduces installation size and time for end users and developers.
  • It reduces the effort of keeping dependencies up to date (including addressing security vulnerabilities).

@nielsfaber nielsfaber merged commit f37290c into nielsfaber:main Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants