Replace Uri.js with urijs npm module #9740

ebogo1 · 2021-08-20T14:28:12Z

This should not be merged until #9706 is in main.

Part of #9473. This code touches a lot of important files and there were a few things that came up; see my review comments.

cesium-concierge · 2021-08-20T14:28:16Z

Thanks for the pull request @ebogo1!

✔️ Signed CLA found.
❔ Changes to third party files were made.
- Looks like a file in one of our ThirdParty folders (ThirdParty/, Source/ThirdParty/) has been added or modified. Please verify that it has a section in LICENSE.md and that its license information is up to date with this new version.

Reviewers, don't forget to make sure that:

Cesium Viewer works.
Works in 2D/CV.
Works (or fails gracefully) in IE11.

ebogo1

For reference, the urijs docs.

ebogo1 · 2021-08-20T14:30:12Z

Source/Core/Resource.js


-    resource._url = uri.resolve(new Uri(getAbsoluteUri(this._url))).toString();
+    if (uri.is("urn")) {


URI.absoluteTo() throws an exception when trying to concatenate URNs, whereas Uri.js returned the unchanged URN. Is this the correct behavior for Resource? There are a couple other places where this came up.

Where exactly are the urn urls coming from?

It's possible in .gltf and .czml files, i.e. the images in https://github.com/CesiumGS/cesium/blob/main/Specs/Data/Models/GltfLoader/BoxTextured/glTF-Embedded/BoxTextured.gltf, used in -

cesium/Specs/Scene/GltfLoaderSpec.js

Line 48 in 1837859

"./Data/Models/GltfLoader/BoxTextured/glTF-Embedded/BoxTextured.gltf";

Not sure how common this is, but it does happen in specs at the very least..

I thought data uris were different than urn?

According to https://stackoverflow.com/a/28865747 urn's start with urn:

Yeah, there's definitely confusion here. almost everything is an urn.. so I'm not sure what differentiates from data uri or other in is

On second thought, I don't think just checking for data uris is enough - ModelOutlineLoaderSpec catches cases in Resource.getDerivedResource where the scheme is "blob". The third party Uri.js in main ignores the base URL's properties (including scheme) if the relative URL has them set:

cesium/Source/ThirdParty/Uri.js

Lines 177 to 182 in 1837859

if (this.scheme) {

uri.scheme = this.scheme;

uri.authority = this.authority;

uri.path = this.path;

uri.query = this.query;

} else {

Sandcastles and specs pass if everywhere that (incorrectly) checks Uri.is("urn") is changed to Uri.scheme() === "". This happens in RequestScheduler.getServerKey, Resource.getDerivedResource, and getAbsoluteUri. Is checking just for data schemes as opposed to checking for any scheme before calling Uri.absoluteTo preferable anywhere? Checking for any scheme at all is more faithful to the behavior in main.

So what is the behavior for these urls in main? Do we just do nothing and return the original url? If so, then we can do the same thing here for now, but it could ultimately be masking a bug. Like should we be trying to absolutize or otherwise manipulate a opaque URL at all?

And wouldn't is("relative") be what we would want to check instead?

It's basically the same check under the hood, but much clearer that we are only doing certain things for relative paths

Do we just do nothing and return the original url?

Yes, this is what the current third party Uri.js does.

And wouldn't is("relative") be what we would want to check instead?

Not quite, since doing so falsely flags urls like "test.png", which causes spec failures. In main, new Uri("test.png").resolve(new Uri("http://localhost:8080/Specs/SpecRunner.html")) turns into "http://localhost:8080/Specs/test.png". This is consistent with this npm module's absoluteTo.

ebogo1 · 2021-08-20T14:33:38Z

Source/Core/getAbsoluteUri.js

+  }
+  var url = relativeUri.absoluteTo(base).toString();
+  // URI.absoluteTo() escapes the placeholders. Undo that.
+  url = url.replace(/%7B/g, "{").replace(/%7D/g, "}");


This code was copied from Resource.prototype.getUrlComponent(); it's needed because the urijs implementation of absoluteTo normalizes URLs (this is also consistent with the URL(relative, base) constructor in JS. Is this logic safe here?

We switched to absoluteTo in a unch of other places as well, why is this the only one that we needed this extra workaround for?

This was a weird case that got caught by this line in specs -

cesium/Specs/Scene/TileMapServiceImageryProviderSpec.js

Line 333 in c24b50f

getAbsoluteUri("made/up/tms/server/{z}/{x}/{reverseY}.jpg")

I don't think it's needed for any other reason..

It almost feels like this fix is in the wrong place, like the calling code is the thing that should be handling { or perhaps a specialized function like getAbsoluteTemplateStringUri.js Feels weird to just have something as generic as getAbsoluteUri embed code like this.

I moved the string replace to the spec in question. Is there no risk of of this mattering at runtime because browsers treat encoded symbols the same as unencoded ones?

I didn't realize the specs were the only place this happens.. did you verify any sandcastles using template uris for imager work?

I verified that imagery Sandcastles work as expected, though there are bunch of Failed to obtain image tile X ... Y: ... Level: ... in the console.. this happens in the same way in main, doesn't seem like we have an open issue for it?

As far as I can tell only the spec calls getAbsoluteUri with a templated url - by the time the imagery urls make their way to fetchImage the template values are already string replaced correctly.

ebogo1 · 2021-08-20T14:35:28Z

Specs/Core/buildModuleUrlSpec.js


 describe("Core/buildModuleUrl", function () {
  it("produces an absolute URL for a module", function () {
    var url = buildModuleUrl("Workers/transferTypedArrayTest.js");

    expect(url).toMatch(/Workers\/transferTypedArrayTest.js$/);
-    expect(new Uri(url).isAbsolute()).toBe(true);
+    var uri = new URI(url);
+    expect(uri.scheme().length).toBeGreaterThan(0);


urijs does not have an equivalent isAbsolute() function so I checked the scheme and fragment manually per the old Uri.js implementation. This change happens again in TaskProcessor.

Specs/DataSources/KmlDataSourceSpec.js

ebogo1 · 2021-08-20T14:38:17Z

Source/Scene/CreditDisplay.js

@@ -521,8 +521,8 @@ function getDefaultCredit() {
      logo.indexOf("https://") !== 0 &&
      logo.indexOf("data:") !== 0
    ) {
-      var logoUrl = new Uri(logo);
-      logo = logoUrl.getPath();
+      var logoUrl = new URI(logo);


This code is never reached by specs..

mramato · 2021-08-22T18:08:04Z

Straight from their README

IMPORTANT: You may not need URI.js anymore! Modern browsers provide the URL and URLSearchParams interfaces.

What missing features are forcing us to use urljs (or any third party for that matter) instead of just using URL and URLSearchParams?

mramato · 2021-08-22T18:08:49Z

@ebogo1 you please merge in npm-third-party-staging (or main if that gets merged before you get to this)

ebogo1 · 2021-08-23T15:24:48Z

What missing features are forcing us to use urljs

@mramato URL expects a valid web URL and throws a type error for things like filepaths - URL docs.

…i-js

mramato · 2021-08-23T17:35:33Z

@mramato URL expects a valid web URL and throws a type error for things like filepaths - URL docs.

I'm not sure what you mean. const url = new URL("file://users/mamato/test.gltf"); works fine.

ebogo1 · 2021-08-23T20:03:19Z

@mramato TaskProcessor is one place that comes to mind; something like new URL("Workers/worker.js") will throw. Maybe less important but URL also does not have a normalize method..

Maybe it should be a follow-up to replace the npm module with stock URL?

mramato · 2021-08-24T03:46:26Z

TaskProcessor is one place that comes to mind; something like new URL("Workers/worker.js") will throw. M

That doesn't sound quite right.

Maybe less important but URL also does not have a normalize method..

MDN specifically mentioned you can just use the href property to normalize the URL.

All this being said, if this is a stop gap and we plan on taking a serious look into using stock URL post release, then I won't hold things up. It could also affect things like Node.js support. But in general it sounds like we assumed too much or gave up to early in trying to use the standard.

Specs/Core/ResourceSpec.js

ebogo1 · 2021-09-01T14:48:56Z

Checking for any scheme at all is more faithful to the behavior in main.

Pushed a change in ca6a116 to replace is("urn") with scheme() !== "".

ebogo1 added 5 commits August 18, 2021 23:18

first pass

a5d7070

replace encoded curly braces in absolute URI

bd7a2e5

check for URN in RequestScheduler

920f722

remove Uri.js

0b9a986

remove Uri.js imports from specs, prettier

8f81fe8

ebogo1 commented Aug 20, 2021

View reviewed changes

ebogo1 added 2 commits August 23, 2021 13:05

Merge remote-tracking branch 'origin/npm-third-party-staging' into ur…

0d2b579

…i-js

fix imports from ThirdPartyNpm

6417168

Base automatically changed from npm-third-party-staging to main August 23, 2021 18:00

ebogo1 mentioned this pull request Aug 23, 2021

Replace third party libraries with npm modules #9473

Closed

29 tasks

ebogo1 marked this pull request as ready for review August 23, 2021 20:07

mramato reviewed Aug 26, 2021

View reviewed changes

Specs/Core/ResourceSpec.js Outdated Show resolved Hide resolved

ebogo1 and others added 10 commits August 29, 2021 11:51

Merge remote-tracking branch 'origin/main' into uri-js

4ed7394

rename to Uri.js

2bed051

removed draco wasm by accident

05d5a13

remove urijs.js

00061e5

Merge remote-tracking branch 'origin/main' into uri-js

233e08a

remove Uri.js from gulpfile

c8287e6

Merge branch 'main' into uri-js

0178356

PR feedback

5c9e139

Merge branch 'uri-js' of github.com:CesiumGS/cesium into uri-js

2d05488

scheme() instead of is(urn)

ca6a116

lilleyse merged commit 5eae657 into main Sep 1, 2021

lilleyse deleted the uri-js branch September 2, 2021 00:42

jjhembd mentioned this pull request Mar 17, 2023

Replace urijs dependency with native URL and URLSearchParams #11168

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace Uri.js with urijs npm module #9740

Replace Uri.js with urijs npm module #9740

ebogo1 commented Aug 20, 2021

cesium-concierge commented Aug 20, 2021

ebogo1 left a comment

ebogo1 Aug 20, 2021

mramato Aug 31, 2021

ebogo1 Aug 31, 2021 •

edited

Loading

lilleyse Sep 1, 2021

mramato Sep 1, 2021

ebogo1 Sep 1, 2021

mramato Sep 1, 2021

mramato Sep 1, 2021

mramato Sep 1, 2021

ebogo1 Sep 1, 2021

ebogo1 Aug 20, 2021

mramato Aug 26, 2021

ebogo1 Aug 26, 2021

mramato Aug 31, 2021

ebogo1 Aug 31, 2021

mramato Sep 1, 2021

ebogo1 Sep 1, 2021

ebogo1 Aug 20, 2021

ebogo1 Aug 20, 2021

mramato commented Aug 22, 2021

mramato commented Aug 22, 2021

ebogo1 commented Aug 23, 2021

mramato commented Aug 23, 2021

ebogo1 commented Aug 23, 2021 •

edited

Loading

mramato commented Aug 24, 2021

ebogo1 commented Sep 1, 2021


		resource._url = uri.resolve(new Uri(getAbsoluteUri(this._url))).toString();
		if (uri.is("urn")) {

	if (this.scheme) {
	uri.scheme = this.scheme;
	uri.authority = this.authority;
	uri.path = this.path;
	uri.query = this.query;
	} else {

Replace Uri.js with urijs npm module #9740

Replace Uri.js with urijs npm module #9740

Conversation

ebogo1 commented Aug 20, 2021

cesium-concierge commented Aug 20, 2021

ebogo1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebogo1 Aug 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mramato commented Aug 22, 2021

mramato commented Aug 22, 2021

ebogo1 commented Aug 23, 2021

mramato commented Aug 23, 2021

ebogo1 commented Aug 23, 2021 • edited Loading

mramato commented Aug 24, 2021

ebogo1 commented Sep 1, 2021

ebogo1 Aug 31, 2021 •

edited

Loading

ebogo1 commented Aug 23, 2021 •

edited

Loading