Fix eslint test

Thank you.

Ok, four problems remain:

  26:35  error  Prefer textContent to $.text  jquery/no-text
/builds/issue/json_field-3450116/web/modules/custom/json_field-3450116/modules/json_field_widget/assets/js/json_widget.js
  44:17  error  Prefer textContent to $.text                 jquery/no-text
  44:32  error  'jsonEditor' was used before it was defined  no-use-before-define
  51:36  error  'JSONEditor' is not defined                  no-undef
✖ 4 problems (4 errors, 0 warnings)

I don't know how to fix these.

I wonder if listing jsonEditor and JSONEditor in the open and closing parts of the wrapper function would help?

Honestly 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.

I wonder if we could load the JS library as a dev dependency via bowser or something?

We're now down to these:

/builds/project/json_field/web/modules/custom/json_field/assets/js/json_field.js
  30:9  error  Insert `··`  prettier/prettier
  31:1  error  Insert `··`  prettier/prettier
/builds/project/json_field/web/modules/custom/json_field/modules/json_field_widget/assets/js/json_widget.js
  47:41  error  'jsonEditor' was used before it was defined  no-use-before-define
  75:36  error  'jsonEditor' is not defined                  no-undef
  75:48  error  'JSONEditor' is not defined                  no-undef
✖ 5 problems (5 errors, 0 warnings)

I don't know what the first two mean? I've never written JS like that.

Prettier 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.

Oh, 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.

Ok, down to one error left:

/builds/project/json_field/web/modules/custom/json_field/modules/json_field_widget/assets/js/json_widget.js
  49:41  error  'jsonEditor' was used before it was defined  no-use-before-define
✖ 1 problem (1 error, 0 warnings)

The tests now pass! Need to manually test it to make sure it still works properly.

In 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.

This was shared via Slack, it might be helpful: https://brianperry.dev/posts/2024/matching-drupal-eslint/

I'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)

It still didn't like that... so instead what if we just pass the options directly into the constructor.

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);

@nicxvan Could you please provide the steps you took to get that error?

Install 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.

Do 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.

I'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.

@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?

I 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.

If 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.

There 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.

I 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.

I 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 🐛 JSON Specific WYSIWYG editor widget does not seem to work. Active

Why 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.

@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?

The 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.

Ok, 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 🐛 JSON Specific WYSIWYG editor widget does not seem to work. Active is also a release blocker because a core feature of the module is broken.

I'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.

Thank 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.

Committed. Thank you.

Automatically closed - issue fixed for 2 weeks with no activity.