Skip to content

Remove unused sourceUri parameter from GeoJsonDataSource #12685

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

khalisahkhan
Copy link

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

  • I have submitted a Contributor License Agreement
  • 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
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

Copy link

Thank you for the pull request, @khalisahkhan!

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

@javagl
Copy link
Contributor

javagl commented Jun 23, 2025

There had been some confusion about this in #6116 - not sure if that is all resolved now.

@mzschwartz5
Copy link
Contributor

mzschwartz5 commented Jul 29, 2025

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.

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

Successfully merging this pull request may close these issues.

3 participants