Closed (fixed)
Project:
JSON Field
Version:
8.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
27 May 2024 at 12:13 UTC
Updated:
13 Feb 2025 at 14:58 UTC
Jump to comment: Most recent
Problem/Motivation
GitLab CI is reporting several eslint errors. Let's fix these.
*****************************************************************************************************************************
These are the current ESLINT errors and warnings
*****************************************************************************************************************************
$ $CI_PROJECT_DIR/$_WEB_ROOT/core/node_modules/.bin/eslint --no-error-on-unmatched-pattern --ignore-pattern="*.es6.js" --resolve-plugins-relative-to=$CI_PROJECT_DIR/$_WEB_ROOT/core --ext=.js,.yml $_ESLINT_EXTRA . || true
/builds/project/json_field/web/modules/custom/json_field/assets/js/json_field.js
7:3 error 'use strict' is unnecessary inside of modules strict
9:3 error Unexpected var, use let or const instead no-var
9:26 error Insert `⏎····` prettier/prettier
13:42 error Insert `,` prettier/prettier
14:8 error Insert `,` prettier/prettier
15:6 error Insert `,` prettier/prettier
24:5 error Expected method shorthand object-shorthand
27:35 error Prefer textContent to $.text jquery/no-text
29:6 error Insert `,` prettier/prettier
30:5 error Delete `⏎` prettier/prettier
/builds/project/json_field/web/modules/custom/json_field/modules/json_field_widget/assets/js/json_widget.js
7:3 error 'use strict' is unnecessary inside of modules strict
12:6 error Delete `⏎···` prettier/prettier
25:60 error Insert `.each(` prettier/prettier
26:9 error Delete `.each(` prettier/prettier
27:11 error Unexpected var, use let or const instead no-var
28:11 error Unexpected var, use let or const instead no-var
29:11 error Unexpected var, use let or const instead no-var
30:11 error Unexpected var, use let or const instead no-var
30:22 error Prefer value to $.val jquery/no-val
31:11 error Unexpected var, use let or const instead no-var
34:13 error All 'var' declarations must be at the top of the function scope vars-on-top
34:13 error Unexpected var, use let or const instead no-var
34:29 error Replace `Drupal.theme('jsonEditorWrapper',·'json-editor-'·+·$textarea.attr('name'))` with `⏎··············Drupal.theme(⏎················'jsonEditorWrapper',⏎················'json-editor-'·+·$textarea.attr('name'),⏎··············),⏎············` prettier/prettier
34:63 error Unexpected string concatenation prefer-template
38:13 error All 'var' declarations must be at the top of the function scope vars-on-top
38:13 error Unexpected var, use let or const instead no-var
41:15 error Expected method shorthand object-shorthand
42:17 error Prefer textContent to $.text jquery/no-text
42:32 error 'jsonEditor' was used before it was defined no-use-before-define
43:16 error Insert `,` prettier/prettier
49:13 error All 'var' declarations must be at the top of the function scope vars-on-top
49:13 error Unexpected var, use let or const instead no-var
49:34 error 'JSONEditor' is not defined no-undef
49:45 error Replace `$editor[0],·Object.assign({},·options,·instanceOptions)` with `⏎··············$editor[0],⏎··············Object.assign({},·options,·instanceOptions),⏎············` prettier/prettier
49:57 error Use an object spread instead of `Object.assign` eg: `{ ...foo }` prefer-object-spread
52:14 error Delete `⏎···········` prettier/prettier
57:10 error Insert `,⏎······` prettier/prettier
58:6 error Insert `,` prettier/prettier
62:12 error Unexpected string concatenation prefer-template
63:4 error Replace `⏎` with `;` prettier/prettier
✖ 40 problems (40 errors, 0 warnings)
Issue fork json_field-3450116
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #3
damienmckennaThank you.
Comment #4
ptmkenny commentedOk, four problems remain:
I don't know how to fix these.
Comment #5
damienmckennaI wonder if listing jsonEditor and JSONEditor in the open and closing parts of the wrapper function would help?
Comment #6
ptmkenny commentedHonestly I never really worked with JQuery so I don't understand what's going on here. All my work recently has been in React/TypeScript, so I'm lost about how to fix these issues.
Comment #7
damienmckennaI wonder if we could load the JS library as a dev dependency via bowser or something?
Comment #8
damienmckennaWe're now down to these:
I don't know what the first two mean? I've never written JS like that.
Comment #9
ptmkenny commentedPrettier just does some basic automatic formatting; it wants you to add whitespace even though it shows the dots.
These kind of fixes can be annoying to do manually, but you can download the patch generated in the build artifacts for the eslint job and apply that, which will automatically fix the things flagged by prettier in most cases.
Comment #10
damienmckennaOh, I thought that meant to add a prefix of ".." in front of a line, not to just indent it. Thanks for the tip of checking the eslint artifacts, I applied the patch, will see what difference it makes.
Comment #11
damienmckennaOk, down to one error left:
Comment #12
damienmckennaThe tests now pass! Need to manually test it to make sure it still works properly.
Comment #13
ptmkenny commentedComment #14
damienmckennaIn local testing this isn't working anymore - the onChange() callback isn't being called. I also had to revert one of the commits to make the editor load in the first place, so clearly the changes were going in the wrong direction.
Comment #15
damienmckennaThis was shared via Slack, it might be helpful: https://brianperry.dev/posts/2024/matching-drupal-eslint/
Comment #17
acbramley commentedI've fixed the last esllint issue, the problem was that onChange was being declared as a function, rather than a property with an anonymous function. That should be the last issue, although I don't know how to manually test all those changes I'm happy to do so to get this merged and unblock 8.x-1.4 (which blocks a stable D11 release)
Comment #18
acbramley commentedIt still didn't like that... so instead what if we just pass the options directly into the constructor.
Comment #19
nicxvan commentedSorry I don't have the time to look into this, but I tried testing this locally.
I pulled this down to test and I get the following error in the console:
Uncaught ReferenceError: jsonEditor is not defined.
) (jQuery, Drupal, drupalSettings, jsonEditor, JSONEditor);
Comment #20
ptmkenny commented@nicxvan Could you please provide the steps you took to get that error?
Comment #21
nicxvan commentedInstall dev
Apply the patch
Change the form mode setting to the editor
Edit a page that has the json field
Check Console
I'm on my phone so I can't check the editor name.
Comment #22
acbramley commentedDo we really need this to block a stable release? If the module works on D11 it'd be great to get a stable release out ASAP and work on CI tweaks afterwards.
Comment #23
nicxvan commentedI'm ok with this not blocking d11, I don't actually use that feature of the module, but to be clear the code editor display doesn't work.
Comment #24
acbramley commented@nicxvan I mean this issue is just about fixing CSS/JS linting errors. Are you saying that the editor doesn't work even without these changes?
Comment #25
nicxvan commentedI don't think the code editor works period. I only tested this issue because it seems to be blocking the d11 release and I saw that bit of js changed.
I think the code editor is a separate issue and would make sense as a follow up.
If you all agree I'm happy to rtbc based on my review let me know.
Comment #26
ptmkenny commentedIf the code editor doesn't currently work, that needs to be opened as a separate issue, and this issue should be made dependent on that one. I don't think it makes sense to commit a code cleanup if the underlying code is broken.
Comment #27
nicxvan commentedThere is no reason to make this dependent, if this gets in, then eslint will enforce the fix is formatted properly anyway.
Unless the maintainers feel differently.
Comment #28
nicxvan commentedI just came back to this, the changes do fix the eslint and I confirmed the issues I found are pre-existing.
RTBC and I'll open an issue for the other issue.
Comment #29
ptmkenny commentedI don't think this can be RTBC when the code that is "fixed" by this isn't working, because there's no way to test if these fixes are valid. Postponing on #3478943: JSON-specific WYSIWYG editor widget does not seem to work
Comment #30
nicxvan commentedWhy would you postpone this, the code editor is broken even without these changes applied.
It has nothing to do with linting so this should be good to merge.
You can test that these "fixes" are valid by reviewing what they are aiming to fix, eslint.
Gonna put this back in rtbc so the maintainers can weigh in.
Comment #31
ptmkenny commented@nicxvan My point is that for automated tests like eslint to "pass", it is not enough for the GitLab CI scan to show no errors. There should be some manual testing where we confirm that behaviors of the module influenced by the code work as expected.
It isn't possible to do such testing when the underlying behavior (the code editor) is broken, so I think this issue should be "postponed"-- otherwise you're committing changes to code that is still broken, and what's the point of that?
Comment #32
nicxvan commentedThe other tests cover the functionality, and as I mentioned I confirmed the bug exists even without these changes.
You commit this one so it unblocks the issues that were depending on it and you resolve the bug in a dedicated issue.
If you merge this in then the issue resolving the code editor bug will also have eslint enforced.
You do not want to introduce new bugs or regressions in an issue, but that is not what is happening here the bug was not introduced with these changes.
Comment #33
ptmkenny commentedOk, I'll just agree to disagree here.
In any case, it seems you want this committed for a stable D11 release, but if this issue is a release blocker, then almost certainly #3478943: JSON-specific WYSIWYG editor widget does not seem to work is also a release blocker because a core feature of the module is broken.
Comment #34
nicxvan commentedI'm happy to chat further on slack, I'm not sure this issue is where to have the discussion.
It's up to the maintainers to decide if it's a blocker to 1.4 release.
However, even core doesn't delay releasing a feature or bug fix because an unrelated bug was discovered.
The feature is broken in the current release, nothing is lost by releasing other features like d11 support. If it were a regression it would be different, but it's not a regression. It's an unrelated bug I discovered and opened an issue for.
Comment #35
damienmckennaThank you for the discussion and for opening the separate issue. Now that we have a good point to work from with the included JS, let's work to fix the other issue as part of this release - people who desperately need the D11 support will have to use the dev release for a little while longer.
Comment #36
damienmckennaCommitted. Thank you.
Comment #38
nicxvan commented