chore: ESLint setup in OpenEMR #6708

raskolnikov-rodion · 2023-07-31T19:46:18Z

Picking up from #3795.

This proposal is supposed to be a more gentle introduction of ESLint into the project. Just laying the foundation. Suggestions on how to make this setup better support the project needs are welcome!

This time, we're only extending the eslint:recommended configuration, with some of the errors turned into warnings. Notable errors include no-undef (1931 occurrences) and no-unused-vars (2405 occurrences).

The quiet option is enabled in the script because otherwise logs would be flooded by warnings. But warnings are still highlighted in IDEs, so we benefit from its checks.

In the future, we can brainstorm strategies to revert those warnings back into errors and extend other configurations, such as the Airbnb one, if desired.

After configuration was done I fixed errors in some files. Some of them I simply disabled the rule as I don't have enough context to judge what would be an appropriate fix. Please take your time and review carefully 🙂

below added by @bradymiller :
Up For Grabs demo for this PR is at (resets to the code in this PR daily at 8:30 am UTC time):
https://www.open-emr.org/wiki/index.php/Development_Demo#Gamma_-_Up_For_Grabs_Demo

package.json

adunsulag

The minified telehealth piece needs to be compiled.

Other than that, my only concern is if you did a testing on generating a ccda document to make sure it is generating those documents properly. The linting shouldn't change the behavior, but its critical to make sure that piece continues to function if the linting slightly altered the behavior.

adunsulag · 2023-08-01T13:46:12Z

ccdaservice/oe-blue-button-generate/lib/headerLevel.js

-    ]
-    , dataKey: "meta.ccda_header.participants"
-}];
+var participants = (exports.participant = [


I'm unfamiliar with this syntax, there's no arrow function here, nor do I see any IFF here. Why are we wrapping this in parenthesis and have you tested the ccda generator with these changes?

My Prettier plugin automatically introduced formatting changes in some of the files before I turned it off. In this case, we have a multi-line expression, so it was wrapped in parenthesis for readability purposes. It does not create any functional impact. You can verify by creating these two files:

// module.js var data = (exports.data = [{ secretKey: '12345'}]); console.log(`data[0].secretKey in module.js: ${data[0].secretKey}`);

// app.js const { data: dt } = require('./module.js'); console.log(`dt[0].secretKey in app.js: ${dt[0].secretKey}`);

And then running node app.js. It should result in the same output, with or without the parenthesis in line 1 of module.js. So they are inoffensive, but I can go back to the file and remove them, if preferred.

No changes needed, thanks for the clarification.

adunsulag · 2023-08-01T13:47:16Z

ccdaservice/oe-blue-button-util/lib/jsonpath/index.js

@@ -113,7 +113,7 @@ var processOptions = function (opts) {
    return {
        resultType: (opts && opts.resultType && opts.resultType.toLowerCase()) || 'value',
        flatten: (opts && opts.flatten) || false,
-        wrap: (opts && opts.hasOwnProperty('wrap')) ? opts.wrap : null,
+        wrap: (opts && Object.prototype.hasOwnProperty.call(opts, 'wrap')) ? opts.wrap : null,


Is the concern here with Object.prototype just to ensure that someone hasn't overridden the hasOwnProperty method?

That's part of the reason. The other part is because the object may not have access to this function, even though it comes from Object.prototype. This happens when you create an object using Object.create(null). Therefore, the operation is deemed unsafe and the linter throws an error.

If you'd like more details you can read here.

Sounds good. Thanks for the clarification.

adunsulag · 2023-08-01T13:48:43Z

gulpfile.js

@@ -429,49 +429,61 @@ function install(done) {
    // combine dependencies and napa sources into one object
    const dependencies = packages.dependencies;
    for (let key in packages.napa) {
-        if (packages.napa.hasOwnProperty(key)) {


I'm assuming you've tested the asset builder to verify the linter didn't break anything?

adunsulag · 2023-08-01T13:51:33Z

...les/custom_modules/oe-module-comlink-telehealth/public/assets/js/src/registration-checker.js

@@ -36,7 +36,7 @@ export function RegistrationChecker(scriptLocation)
                return result.json();
            })
            .then(registrationSettings => {
-                if (registrationSettings && registrationSettings.hasOwnProperty('errorCode')) {
+                if (registrationSettings && Object.prototype.hasOwnProperty.call(registrationSettings, 'errorCode')) {


If you are going to change these values you need to rebuild the minified json in the telehealth src using

cd ./openemr/interface/modules/custom_modules/oe-module-comlink-telehealth/public/assets/js/ npm run build

raskolnikov-rodion · 2023-08-01T18:03:31Z

The minified telehealth piece needs to be compiled.

Other than that, my only concern is if you did a testing on generating a ccda document to make sure it is generating those documents properly. The linting shouldn't change the behavior, but its critical to make sure that piece continues to function if the linting slightly altered the behavior.

Hey @adunsulag thanks for your review! I tested running the scripts contained in the README file

composer install --no-dev
npm install
npm run build
composer dump-autoload -o

and I got no errors. If I'm not mistaken this also tests the gulpfile too, right?

I'm new to the project so I don't know how to test generating a ccda document, so if you (or anyone else) can give me some pointers on how to do that I'll be able to report back and it'd be much appreaciated.

I rebuilt the telehealth file, will push it right away. Let me know if there's anything else I can do.

stephenwaite · 2023-08-02T14:19:08Z

hi @raskolnikov-rodion , check out the Creating Random Patients Video Tutorial which will create ccdas in the process of just amazing fun along the way :)

bradymiller · 2023-08-07T02:59:51Z

Up For Grabs demo for this PR is at (resets to the code in this PR daily at 8:30 am UTC time):
https://www.open-emr.org/wiki/index.php/Development_Demo#Gamma_-_Up_For_Grabs_Demo

bradymiller · 2023-08-07T03:09:29Z

btw, the alpine 3.18 docker demo care coordination module is breaking since ccda build broken, so is broken on the up for grabs demo. This is not your issue @raskolnikov-rodion . Looking into it (for some reason, then alpine 3.18 demo docker is bringing in nodejs 20 instead of 18 whereas the production/flex alpine 3.18 dockers are correctly bringing in 18).

bradymiller · 2023-08-07T05:48:36Z

Above alpine 3.18 demo is fixed (odd issue where if do not explicitly bring in nodejs package, the npm packages brings in nodejs 20 instead of nodejs 18: openemr/demo_farm_openemr@4a7ce96), so the Care Coordination module (Modules->Carecoordination in menu) now works.

bradymiller · 2023-08-07T05:51:35Z

hi @raskolnikov-rodion , Would be very neat to have this js lint. Would it make sense to run it like do the css lint stuff where can then run it as a github action?

bradymiller · 2023-08-07T05:52:51Z

Also, is this able to do autocorrect for certain rules (like the css lint and php lint do?)?

bradymiller · 2023-08-07T05:56:02Z

One more question. Is this able to check embedded js in the .php scripts?

raskolnikov-rodion · 2023-08-07T17:46:44Z

hi @raskolnikov-rodion , check out the Creating Random Patients Video Tutorial which will create ccdas in the process of just amazing fun along the way :)

Hi @stephenwaite , thanks for sharing the link to the tutorial, it was really helpful! I was able to follow all the steps and generate 10 random patients as described, with no apparent errors in the process 🙂

raskolnikov-rodion · 2023-08-07T18:24:17Z

Hi @bradymiller , thanks for setting up the demo for the pull request!

Would it make sense to run it like do the css lint stuff where can then run it as a github action?

Yes! I just added a step for ESLint in styling.yml file, right after the stylelint check, following the same pattern. See if it makes sense and let me know if you have any suggestions.

Also, is this able to do autocorrect for certain rules (like the css lint and php lint do?)?

Yes, ESLint has a --fix option just like the other linters, which is able to fix some errors automatically. If there's interest we could even setup some hooks to run these lint fixes before commiting or pushing.

One more question. Is this able to check embedded js in the .php scripts?

I don't know, as I have never used ESLint with PHP files. But it could be worth looking into it. A quick search on npm gives this plugin, so it may be possible. I'm thinking we finish the setup for JS files in this PR, and extend it for PHP in a future PR, so we keep changes small and focused. What do you think?

bradymiller · 2023-08-09T03:27:01Z

bradymiller · 2023-08-09T03:27:59Z

@bradymiller , note to self. incorporate this (and --fix command) in the devtools in the docker dev environments

UPDATE, PRs are here:
#6719
openemr/openemr-devops#363

stephenwaite reviewed Jul 31, 2023

View reviewed changes

package.json Outdated Show resolved Hide resolved

adunsulag requested changes Aug 1, 2023

View reviewed changes

raskolnikov-rodion requested a review from adunsulag August 1, 2023 18:24

raskolnikov-rodion added 18 commits August 7, 2023 19:13

chore(eslint): ESLint setup

4458a1b

fix(portal): fix lint error in signer_api

892d8a6

fix(library): disable no-constant-condition error in textformat

fd2d206

fix(library): disable no-cond-assign error in validate_extend

8868d51

fix(library): disable no-inner-declarations error in utility

f9d1604

fix(library): fix lint error in html

8a2ceca

fix(library): fix lint errors in clickmap

34ad41d

fix(library): fix lint errors in ajax_functions_writer

5218cf6

fix(library): fix lint errors in dialog

dcb514f

fix(interface): fix lint errors in product_registration_service

d357f55

fix(interface): disable no-unused-labels in autosuggest

4574e15

fix(interface): fix lint errors in sendTo

fb4d243

fix(interface): fix lint errors in oe-module-comlink-telehealth

fc70844

fix(interface): fix lint errors in tabs

c0517d4

fix(interface): fix lint errors in fee_sheet_core

f165b61

fix(gulp): fix lint errors in gulpfile

efec70d

fix(ccdaservice): fix lint errors

739addd

chore(eslint): pin package versions

45b594c

raskolnikov-rodion added 2 commits August 7, 2023 19:13

chore(interface): rebuild telehealth.min.js

0b4db12

feat(ci): add step for ESLint

854ee8e

raskolnikov-rodion force-pushed the chore/eslint-setup branch from c77f31e to 854ee8e Compare August 7, 2023 18:24

adunsulag approved these changes Aug 8, 2023

View reviewed changes

bradymiller merged commit 7de6b61 into openemr:master Aug 9, 2023

raskolnikov-rodion deleted the chore/eslint-setup branch August 10, 2023 08:03

adunsulag changed the title ~~chore(eslint): ESLint setup~~ chore: ESLint setup in OpenEMR Nov 16, 2023

adunsulag added this to the 7.0.2 milestone Nov 16, 2023

adunsulag added the developers This issue targets an issue that is for developers/collaborators/module writers/technical users label Nov 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: ESLint setup in OpenEMR #6708

chore: ESLint setup in OpenEMR #6708

raskolnikov-rodion commented Jul 31, 2023 •

edited by bradymiller

Loading

adunsulag left a comment

adunsulag Aug 1, 2023

raskolnikov-rodion Aug 1, 2023 •

edited

Loading

adunsulag Aug 8, 2023

adunsulag Aug 1, 2023

raskolnikov-rodion Aug 1, 2023

adunsulag Aug 8, 2023

adunsulag Aug 1, 2023

adunsulag Aug 1, 2023

raskolnikov-rodion commented Aug 1, 2023

stephenwaite commented Aug 2, 2023

bradymiller commented Aug 7, 2023

bradymiller commented Aug 7, 2023

bradymiller commented Aug 7, 2023

bradymiller commented Aug 7, 2023 •

edited

Loading

bradymiller commented Aug 7, 2023 •

edited

Loading

bradymiller commented Aug 7, 2023

raskolnikov-rodion commented Aug 7, 2023

raskolnikov-rodion commented Aug 7, 2023

bradymiller commented Aug 9, 2023

bradymiller commented Aug 9, 2023 •

edited

Loading

chore: ESLint setup in OpenEMR #6708

chore: ESLint setup in OpenEMR #6708

Conversation

raskolnikov-rodion commented Jul 31, 2023 • edited by bradymiller Loading

adunsulag left a comment

Choose a reason for hiding this comment

adunsulag Aug 1, 2023

Choose a reason for hiding this comment

raskolnikov-rodion Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

adunsulag Aug 8, 2023

Choose a reason for hiding this comment

adunsulag Aug 1, 2023

Choose a reason for hiding this comment

raskolnikov-rodion Aug 1, 2023

Choose a reason for hiding this comment

adunsulag Aug 8, 2023

Choose a reason for hiding this comment

adunsulag Aug 1, 2023

Choose a reason for hiding this comment

adunsulag Aug 1, 2023

Choose a reason for hiding this comment

raskolnikov-rodion commented Aug 1, 2023

stephenwaite commented Aug 2, 2023

bradymiller commented Aug 7, 2023

bradymiller commented Aug 7, 2023

bradymiller commented Aug 7, 2023

bradymiller commented Aug 7, 2023 • edited Loading

bradymiller commented Aug 7, 2023 • edited Loading

bradymiller commented Aug 7, 2023

raskolnikov-rodion commented Aug 7, 2023

raskolnikov-rodion commented Aug 7, 2023

bradymiller commented Aug 9, 2023

bradymiller commented Aug 9, 2023 • edited Loading

raskolnikov-rodion commented Jul 31, 2023 •

edited by bradymiller

Loading

raskolnikov-rodion Aug 1, 2023 •

edited

Loading

bradymiller commented Aug 7, 2023 •

edited

Loading

bradymiller commented Aug 7, 2023 •

edited

Loading

bradymiller commented Aug 9, 2023 •

edited

Loading