Frontend: Various improvements / Development experience

[pierre-gilles]

Hi Gladys developers

The Gladys 4 « frontend » project was created in 2018/2019, and some configuration points are becoming outdated/more suitable in hindsight.

I’d like us to discuss together the different things that can be updated so the code is more up to date, and to iterate on what worked/didn’t work in the frontend.

Update ESLint

Update the ESLint configuration to allow the latest JS ES2023 improvements (the ?. operator for example, we talked about it with @bertrandda on his last PR)

This makes me think we should do it on the backend too because some errors related to JSDoc are impossible to understand, and in a newer version it’s much more comprehensible!

Stop using the @connect decorators used by unistore

That was trendy at the time and in the end unistore no longer recommends it; it was a « hack » allowed by the transpiler, I don’t even think it’s in the JS spec in 2023.

Before:

Now:

I need to add an ESLint rule to ban this behavior.

Heavily restrict the use of the global unistore store

I’ll admit it’s my fault — at the start of v4 I thought a global store would be a good idea, probably due to lack of experience at the time with React frontend development. The result: we had many bugs related to the global store, because variables inevitably polluted each other between components.

My recommendation: Limit the store usage to « global » variables: user, session, httpClient, and that’s it.

The rest should be class states, local to the class. That avoids pollution between components.

Also, we should see if ESLint could help warn the developer.

Unify style behavior

Styling in the frontend hasn’t necessarily been enforced at the ESLint level, and in my opinion that’s a mistake.

Some components use inline styles (style={{}}), which is a bad practice performance-wise because an object is recreated on every render.

Also, those styles are hard to modify for responsive handling.

My recommendation: .css style files (one per feature roughly), which are imported into the component.

Media queries become possible and allow handling small/large devices:

More ESLint rules to speed up PR reviews

I realized while reviewing @bertrandda’s last PR that I often made the same comments on frontend code in PRs (watch out for inline functions, inline styles, etc.), and that these feedbacks could be in ESLint so reviews go faster

@contributors: What do you think? Any other feedback on the frontend?

[pierre-gilles]

So I got going today — I updated server-side ESLint, particularly on the JSDoc side

Upgrade & fix eslint jsdoc server side

It’s a lot of changes, but the errors are much clearer now

[pierre-gilles]

I started a PR to add a warning about using the @connect decorator (and replace it with the function call)

113 warnings so far:

Painstaking work but I’m making quick progress

Upgrade front Eslint, remove all @connect decorator

[pierre-gilles]

So for now I’ve added 2 rules :

1. A warning for inline styles
2. An error for the @connect decorators
3. For later, add a big warning on the use of store.setState to encourage the use of local setState in components

Everything is fixed in this PR : Upgrade front Eslint, remove all @connect decorator by Pierre-Gilles · Pull Request #1766 · GladysAssistant/Gladys · GitHub

[pierre-gilles]

I merged these 2 PRs !

Those who are working on Gladys PRs right now (@AlexTrovato, @bertrandda, @Pti_Nico, @euguuu, @Romuald_Pochet ), I recommend that you rebase your branches, run an npm install (server/front), and run ESLint to check that your code is up to date

If you have any trouble making the changes, don’t hesitate !

[AlexTrovato]

Great, I support what you’re doing.
Now it’s up to us to make sure we fix the warnings one integration at a time — or are you up for it and will you take care of it, @pierre-gilles?

[Romuald_Pochet]

Good evening,

I started to include the changes in the zwave-js-ui integration and I have an issue with the following method:

/**
* @description Return array of Nodes.
* @param {string} orderDir - Ordering.
* @param {string} search - Keyword to match.
* @returns {Array} Return list of nodes.
* @example
* const nodes = zwaveManager.getNodes();
function getNodes({ orderDir = undefined, search = undefined } = {}) {

After updating ESLint, I get the following error:

  30:1  error  @param "search" does not match an existing function parameter  jsdoc/check-param-names

Any idea?

[pierre-gilles]

@Romuald_Pochet Yes, those aren’t function parameters you’re describing, it’s an object!

So you should do it like this:

Also, small feedback, the = undefined isn’t necessary, because if you don’t specify an orderDir, it’ll be undefined anyway

[pierre-gilles]

@AlexTrovato

Now it’s up to us to make sure we fix the warnings integration by integration, or are you up for taking care of it @pierre-gilles?

I fixed everything in the two PRs yesterday

The only thing I didn’t do was enable the ESLint rule to add a warning for uses of unistore (file actions.js), because it would create 600 warnings

In the future, try to stop using those actions.js files in favor of local state, and if you have some time, gradually migrate existing components After all, it’s not a priority — it’s more of a guideline for the future

[AlexTrovato]

I was talking about helping to fix the 146 front-end warnings, mainly about the use of the « style » attribute: « Using inline style is not recommended. Please use a .css file » There are also the promise warnings for « Prefer await to then()/catch()/finally() »

[pierre-gilles]

Ah indeed! Yes, I’d welcome help on those, but for me it’s not a priority to do everything at once — we can do it progressively over time

For me there are other development priorities right now, typically the Zigbee project you started to choose the brand of USB stick;