Issue10352 removegetvariables

[PieterL75]

Fixes: #10352 Remove empty variables from GET urls

[PieterL75]

The script works, but I fail to get it to work with the typescript files. Could you check what I'm doing wrong ?

[kkthxbye-code]

Not sure why lldp.js is changed, I don't think it should with the current changes. The relevant dist files should be updated before merge though.

Your current approach needs some work, please see comments.

[jeremystretch]

Works well for me. @kkthxbye-code any lingering concerns?

[kkthxbye-code]

@jeremystretch - I don't think the approach works in general. It breaks the form when navigating back as the elements are still disabled, at least for me in both chrome and firefox.

Maybe an alternative approach would be to modify the history with window.history.pushState()/window.history.replaceState(). I'm not really a frontend guy though.

The new code also has some other issue. The switch is not needed for two outcomes, there should not be a default case as that will break if we ever add other form elements to a get form, there's redunant checks when checking for falsy values, the function returns a boolean for no reason, the prototype of the object is compared by the nodeName string instead of Object.getPrototypeOf.

[PieterL75]

I'm not really a frontend guy though.
Me neither :-D

The switch is not needed for two outcomes, there should not be a default case as that will break if we ever add other form elements to a get form
I had to change the element type to HTMLSelectElement when using the TypeScript, In regular Javascript it just ignores the fact that the selectedItems does not exist.

there's redunant checks when checking for falsy values
Indeed. I had to do that to catch all cases. Just comparing to only null, empty string, ... did not catch all 'empty' cases

the function returns a boolean for no reason
In my experience, the 'onsubmit' event function has to return a true or false. In case of false the form submit is canceled.

the prototype of the object is compared by the nodeName string instead of Object.getPrototypeOf.
I see that Object.getPrototypeOf return the class, but I have no idea how work with it to typescript validating properly

[kkthxbye-code]

I had to change the element type to HTMLSelectElement when using the TypeScript, In regular Javascript it just ignores the fact that the selectedItems does not exist.

I realize that, but you didn't have to treat everything else as an HTMLInputElement.

Indeed. I had to do that to catch all cases. Just comparing to only null, empty string, ... did not catch all 'empty' cases

!inputElement.value ||
inputElement.value == null ||
inputElement.value == ''

The first !inputElement.value covers the last two cases in your code here. That's what I meant.

In my experience, the 'onsubmit' event function has to return a true or false. In case of false the form submit is canceled.

I didn't see that in my testing. Returning false might break it, but when I tested making the function return void did not.

I see that Object.getPrototypeOf return the class, but I have no idea how work with it to typescript validating properly

Just validate first that Object.getPrototypeOf(element) === HTMLInputElement.prototype, then cast it to the type.

Regardless, as I said, I think breaking back button behavior is a no-go here, so the chosen approach seems to be a non-starter.

[jeremystretch]

I don't think the approach works in general. It breaks the form when navigating back as the elements are still disabled, at least for me in both chrome and firefox.

I'm inclined to agree. I suspect we'd need to handle the form submission ourselves, ignoring empty values, rather than relying on the default submission process. But I'm also not a front end person. 😆

[PieterL75]

Well, you can't say I didn't try :-D

Another option that I read, is to use the FormData, going to test that out

[jsenecal]

We could preventDefault() and submit the form via get only the fields with data manually.

Is that your intent @PieterL75 ?

[PieterL75]

https://developer.mozilla.org/en-US/docs/Web/API/FormDataEvent

  const documentForms = document.forms
  for (var documentForm of documentForms) {
    if (documentForm.method.toUpperCase() == 'GET') {
      documentForm.addEventListener('formdata', function(event) {
        let formData = event.formData;
        for (let [name, value] of Array.from(formData.entries())) {
          if (value === '') formData.delete(name);
        }
      });
    }
  }

src: https://stackoverflow.com/questions/8029532/how-to-prevent-submitting-the-html-forms-input-field-value-if-it-empty/64029534#64029534

This works pretty awesome.. too bad typescript doesnt know the formdata event

[kkthxbye-code]

Works as expected, see suggested change.

[kkthxbye-code]

I created #10526 to test if my suggestion really was only working for me, but it seems like the CI builds it fine, so you should check your environment to make sure it is up to date.

You can see the change here:

https://github.com/netbox-community/netbox/pull/10526/files

I would prefer if you could try again to make sure we do it properly. Sorry for all the issues, the solution you came up with is great!

[PieterL75]

but it seems like the CI builds it fine, so you should check your environment to make sure it is up to date.

It's actually my local tsc (v4.8.4, latest build) that is throwing the errors.
I can override it (as in, disable the tsc check) for this build, but not sure how it will handle it in future builds

$ tsc --noEmit
src/netbox.ts(46,28): error TS2339: Property 'formData' does not exist on type 'Event'.
src/netbox.ts(47,16): error TS2488: Type 'unknown' must have a '[Symbol.iterator]()' method that returns an iterator.

[kkthxbye-code]

You are running a globally installed tsc. You need to use yarn validate instead, which uses the correct typescript version installed locally.

Docs link: https://demo.netbox.dev/static/docs/development/web-ui/

[PieterL75]

I did follow that guide to setup all things.. worked perfect till now :-(
its part of the output of the pre-commit https://docs.netbox.dev/en/stable/development/getting-started/#3-enable-pre-commit-hooks, so not ran manually..
yarn v1.22.19, tsc v4.8.4
Not sure what is causing the difference in interpretation by the CI build and the pre-commit script.
What versions are the CI builds running ?

But I think I really need the 3 comments, as per the docs:
A // @ts-ignore comment suppresses all errors that originate on the following line. It is recommended practice to have the remainder of the comment following @ts-ignore explain which error is being suppressed.

[kkthxbye-code]

Remove the typescript you have globally installed with something like.

yarn global remove typescript

No you don't need ignore on all lines, please see the CI build in the PR I linked, it runs validate fine. Nevertheless you need to fix your environment, you are currently using a newer version of typescript than netbox.

Edit:

I just tested with a fresh node docker image, where I the global typescript package and it always prefers the project installed one, so I'm not sure how your environment got so messed up. You can try running

yarn list|grep typescript@

The output should include:

├─ [email protected]

We are getting a little bit into tech support here though.