Cockpit location ts #21276

jelly · 2024-11-15T17:08:23Z

remove arguments use
Property 'replaceAll' does not exist on type 'string'.
Rethink API, singleton?
Solve A property name to write to depends on a user provided value
solve

# pkg/lib/cockpit/location.ts:168:57 - error TS2322: Type 'string | string[]' is not assignable to type 'string'.
#   Type 'string[]' is not assignable to type 'string'.
#
# 168                                 last = options[name] = [last];
#                                                             ~~~~

pkg/lib/cockpit/location.ts

+                        if (options[name]) {
+                            let last = options[name];
+                            if (!Array.isArray(value))
+                                last = options[name] = [last];


pkg/lib/cockpit/location.ts

jelly · 2024-11-18T10:07:41Z

Locally I have this test failure to cleanup:

Expected: 	"#/other"
Result: 	"#/not-gonna-happen"

    window.setTimeout(function() {
        location.go(["not-gonna-happen"]);
        assert.strictEqual(window.location.hash, "#/other", "hash is correct");

        cockpit.location.go(["gonna-happen"]);
        assert.strictEqual(window.location.hash, "#/gonna-happen", "hash has changed");

        done();
    }, 100);

    /* User or something else navigates */
    window.location.hash = "#/other";

jelly · 2024-11-20T12:03:03Z

So in to sum this all up, the test code is:

window.location.hash = "#/hello";

const location = cockpit.location;
assert.deepEqual(cockpit.location.path, ["hello"], "path is right");

window.setTimeout(function() {
  location.go(["not-gonna-happen"]);
  assert.strictEqual(window.location.hash, "#/other", "hash is correct");

  cockpit.location.go(["gonna-happen"]);
  assert.strictEqual(window.location.hash, "#/gonna-happen", "hash has changed");
done();
}, 100);

/* User or something else navigates */
window.location.hash = "#/other";

More verbose test output is by adding a console.log to replace and go as both work with last_loc

test - path is right
self -> Location, last_loc => null
test hash is correct
self -> Location, last_loc => Location
test hash has changed

So last_loc is our last cached location. This is initialized when cockpit.location is queried and set to null when the hashchange window event happens OR re-initialised when !last_loc || last_loc.href !== get_window_location_hash().

So this is why our tests fail. That leaves the question to why does this all happen.

pkg/lib/cockpit/location.ts

Prepare for moving cockpit.location to a separate typescript file named location.

Apparently String.prototype.replaceAll is an 2021 feature.

This function also allows `string[]` as can be seen by looking at how the code is used in our code.

pkg/lib/cockpit/location.ts

+                                last = options[name] = [last];
+                            last.push(value);
+                        } else {
+                            options[name] = value;


cockpituous · 2024-11-26T17:26:25Z

pkg/lib/cockpit/location.ts

+        if (window.mock?.url_root)
+            this.url_root = window.mock.url_root;


These 2 added lines are not executed by any test.

cockpituous · 2024-11-26T17:26:25Z

pkg/lib/cockpit/location.ts

+        if (application.indexOf("cockpit+=") === 0) {
+            if (this.url_root)
+                this.url_root += '/';
+            this.url_root = this.url_root + application.replace("cockpit+", '');


These 4 added lines are not executed by any test.

cockpituous · 2024-11-26T17:26:25Z

pkg/lib/cockpit/location.ts

+            } else if (part == "..") {
+                if (out.length === 0)
+                    return [];
+                out.pop();


These 4 added lines are not executed by any test.

cockpituous · 2024-11-26T17:26:25Z

pkg/lib/cockpit/location.ts

+        } else if (path instanceof Location) {
+            return path.href;


These 2 added lines are not executed by any test.

cockpituous · 2024-11-26T17:26:25Z

pkg/lib/cockpit/location.ts

+
+    decode(href: string, options: Options) {
+        if (href[0] == '#')
+            href = href.substring(1);


This added line is not executed by any test.

cockpituous · 2024-11-26T17:26:25Z

pkg/lib/cockpit/location.ts

+                        if (options[name]) {
+                            let last = options[name];
+                            if (!Array.isArray(last))
+                                last = options[name] = [last];
+                            last.push(value);


These 5 added lines are not executed by any test.

cockpituous · 2024-11-26T17:26:25Z

pkg/lib/cockpit/location.ts

+    toString() {
+        return this.href;


These 2 added lines are not executed by any test.

jelly added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Nov 15, 2024

github-advanced-security bot found potential problems Nov 15, 2024

View reviewed changes

jelly force-pushed the cockpit-location-ts branch from 4ebff51 to 547af93 Compare November 20, 2024 12:27

github-advanced-security bot found potential problems Nov 20, 2024

View reviewed changes

pkg/lib/cockpit/location.ts Fixed Show fixed Hide fixed

pkg/lib/cockpit/location.ts Fixed Show fixed Hide fixed

allisonkarlitskaya self-requested a review November 20, 2024 21:16

jelly added 7 commits November 26, 2024 17:24

lib: cockpit: rename location to location-utils

bd8e4d5

Prepare for moving cockpit.location to a separate typescript file named location.

lib: cockpit: split location into a separate class

dd37ca1

base1: explain cockpit.location hashchange test

eca3770

lib: fixups/ignore TS

09189b1

tsconfig: bump es version to 2021

431d88c

Apparently String.prototype.replaceAll is an 2021 feature.

lib: correctly type cockpit.location.{go,replace}

0ce12ab

This function also allows `string[]` as can be seen by looking at how the code is used in our code.

fullly lint

9860d1c

jelly removed the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Nov 26, 2024

jelly force-pushed the cockpit-location-ts branch from 547af93 to 9860d1c Compare November 26, 2024 16:30

github-advanced-security bot found potential problems Nov 26, 2024

View reviewed changes

pkg/lib/cockpit/location.ts

last = options[name] = [last];

last.push(value);

} else {

options[name] = value;

Check failure

Code scanning / CodeQL

Remote property injection High

A property name to write to depends on a

user-provided value

Loading
.

cockpituous reviewed Nov 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cockpit location ts #21276

Cockpit location ts #21276

jelly commented Nov 15, 2024 •

edited

Loading

jelly commented Nov 18, 2024

jelly commented Nov 20, 2024

cockpituous Nov 26, 2024

cockpituous Nov 26, 2024

cockpituous Nov 26, 2024

cockpituous Nov 26, 2024

cockpituous Nov 26, 2024

cockpituous Nov 26, 2024

cockpituous Nov 26, 2024

		if (window.mock?.url_root)
		this.url_root = window.mock.url_root;

Cockpit location ts #21276

Are you sure you want to change the base?

Cockpit location ts #21276

Conversation

jelly commented Nov 15, 2024 • edited Loading

jelly commented Nov 18, 2024

jelly commented Nov 20, 2024

cockpituous Nov 26, 2024

Choose a reason for hiding this comment

cockpituous Nov 26, 2024

Choose a reason for hiding this comment

cockpituous Nov 26, 2024

Choose a reason for hiding this comment

cockpituous Nov 26, 2024

Choose a reason for hiding this comment

cockpituous Nov 26, 2024

Choose a reason for hiding this comment

cockpituous Nov 26, 2024

Choose a reason for hiding this comment

cockpituous Nov 26, 2024

Choose a reason for hiding this comment

jelly commented Nov 15, 2024 •

edited

Loading