Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace urijs dependency with native URL and URLSearchParams #11168

Open
jjhembd opened this issue Mar 17, 2023 · 2 comments
Open

Replace urijs dependency with native URL and URLSearchParams #11168

jjhembd opened this issue Mar 17, 2023 · 2 comments

Comments

@jjhembd
Copy link
Contributor

jjhembd commented Mar 17, 2023

The urijs repository is no longer active. The README recommends switching to URL and URLSearchParams.

Challenges

We have discussed this before, and noticed one complication for the switch:

URL expects a valid web URL and throws a type error for things like filepaths

The error should usually be avoidable by supplying a base path when constructing a URL from a relative path:

const url = new URL(relativePath, location);

location should be a suitable base URL in most cases, since it is even defined in Workers. But some situations like parsing KMLs may require more thought.

Performance Implications

Unfortunately, switching to the native API may not provide any performance benefit. If anything, the native API is slightly slower. See this rough benchmark.

URI can already be a significant performance hit for some datasets, so a PR to replace it will need to be thoroughly performance tested.

@jjhembd
Copy link
Contributor Author

jjhembd commented Apr 13, 2023

One place where we are currently leaning on relative URIs is in the ImplicitTileset constructor. The subtree and content URI templates are constructed without any base path:

  this.subtreeUriTemplate = new Resource({ url: implicitTiling.subtrees.uri });
  // ...
    const contentResource = new Resource({ url: contentHeader.uri });

These URIs are often specified in the tileset.json file as relative URIs, and according to the spec,

When the URI is relative, its base is always relative to the referring tileset JSON file.

Within the ImplicitTileset constructor, we have the URL of the "referring tileset JSON file" via the baseResource parameter, so we could simply do this:

  this.subtreeUriTemplate = baseResource.getDerivedResource({ url: implicitTiling.subtrees.uri });
  // ...
    const contentResource = baseResource.getDerivedResource({ url: contentHeader.uri });

Note that this would copy all the Resource constructor options (queryParameters, templateValues, headers, etc) from the baseResource, instead of using the defaults as currently.

@jjhembd
Copy link
Contributor Author

jjhembd commented Apr 21, 2023

  this.subtreeUriTemplate = baseResource.getDerivedResource({ url: implicitTiling.subtrees.uri });
  // ...
    const contentResource = baseResource.getDerivedResource({ url: contentHeader.uri });

This works. With a few additional changes, all specs related to Cesium3DTileset can pass, with URI.js replaced by URL in Resource.

Another place where we lean on a non-standard handling of relative URIs is in KmlDataSource. The spec "NetworkLink: within a kmz file" reads the file Specs/Data/KML/multilevel.kmz, which has the following structure:

doc.kml
Level1/
Level1/doc.kml
Level1/Level2/
Level1/Level2/doc.kml

To read the different levels correctly, the relative file path must be preserved, because it is used as a lookup key into the ZIP file via uriResolver. I didn't see an easy solution on first look—the dataflow in KmlDataSource is fairly complicated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant