cesium icon indicating copy to clipboard operation
cesium copied to clipboard

Remove unused sourceUri parameter from GeoJsonDataSource

Open khalisahkhan opened this issue 6 months ago • 3 comments

Description

Removed the sourceUri parameter from the method signature and internal function calls in GeoJsonDataSource.load().

  • Cleaned up unused imports (e.g., getFilenameFromUri).
  • Updated inline documentation to reflect the change.

The sourceUri parameter was no longer used in the logic and acted as a no-op. Removing it improves code clarity and reduces potential confusion for contributors and users of the API.

Issue number and link

Issue #6108 by removing the unused sourceUri parameter from the GeoJsonDataSource.load() method.

Testing plan

Verified that:

  • The app builds successfully using npm start
  • GeoJsonDataSource.load() continues to work without the sourceUri parameter
  • No regressions occur in runtime behavior

Author checklist

  • [x] I have submitted a Contributor License Agreement
  • [x] I have added my name to CONTRIBUTORS.md
  • [ ] I have updated CHANGES.md with a short summary of my change
  • [ ] I have added or updated unit tests to ensure consistent code coverage
  • [x] I have updated the inline documentation, and included code examples where relevant
  • [x] I have performed a self-review of my code

khalisahkhan avatar Jun 22 '25 20:06 khalisahkhan

Thank you for the pull request, @khalisahkhan!

:white_check_mark: We can confirm we have a CLA on file for you.

github-actions[bot] avatar Jun 22 '25 20:06 github-actions[bot]

There had been some confusion about this in https://github.com/CesiumGS/cesium/pull/6116 - not sure if that is all resolved now.

javagl avatar Jun 23 '25 12:06 javagl

Hi @khalisahkhan, thanks for taking a look at this issue. Reading over this comment from the first PR that attempted to fix this, it seems like there may be some unintuitive behavior in this class that we're not addressing.

Specifically, it seems like we need to support an edge case where the user passes in a data URI geoJson to load, and not an object with a name (if I'm correctly understanding the comment I linked).

From that first PR, there's a unit test added:

  it("load works with a URL", function () {
    const dataSource = new GeoJsonDataSource();
    return dataSource.load("Data/test.geojson").then(function () {
      expect(dataSource.name).toEqual("test.geojson");
    });
  });

This test passes with the current GeoJsonDataSource class, but not after the changes in this PR - removing the sourceURI and using geoJson.name. That tells me we're still missing something.

Honestly, though, I'm sort of wondering if we should just let the existing code be - after all, it works, and it's not obviously wrong in any way.

mzschwartz5 avatar Jul 29 '25 17:07 mzschwartz5