search

nielsfaber / scheduler-card

HA Lovelace card for control of scheduler entities
GNU General Public License v3.0
880 stars 111 forks source link

Run prettier and lint #806

Closed pdcastro closed 6 months ago

pdcastro commented 6 months ago

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
> scheduler-card@1.0.0 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?

nielsfaber commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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: