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

Rd-324: Handle invalid map style in constructor #121

Closed
wants to merge 3 commits into from
Closed

Conversation

jonathanlurie
Copy link
Collaborator

@jonathanlurie jonathanlurie commented Oct 3, 2024

RD-324

If an invalid style URL or shorthand is provided to the Map constructor or to the .setStyle(...) method it was failing without any message other than the Maplibre HTTP error in the console.

Now, if the style leads to a URL with an HTTP status other than 2xx, it falls back to the built-in STREETS style and displays a message in the console.

It can be tested with

const map = new maptilersdk.Map({
  container: "map-container",
  style: "streets-v100", // this one does not exist
});

// or

const map = new maptilersdk.Map({
  container: "map-container",
  style: "00000000-0000-0000-0000-000000000000", // a wrong UUID that has the shape of a real onw
});

// or 

const map = new maptilersdk.Map({
  container: "map-container",
  style: "/some-inesisting-style.json", // a local URL pointing to something inexisting
});

Note: Using an invalidReferenceMapStyle or MapStyleVariant such as MapStyle.STREEEETS or MapStyle.STREETS.DAAAARK is a situation that was already dealt with by using a fallback.

@jonathanlurie jonathanlurie requested review from petrsloup and removed request for petrsloup October 3, 2024 13:50
@jonathanlurie jonathanlurie marked this pull request as draft October 4, 2024 07:41
@jonathanlurie jonathanlurie self-assigned this Oct 4, 2024
@jonathanlurie jonathanlurie marked this pull request as ready for review October 4, 2024 14:44
Copy link
Member

@petrsloup petrsloup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't there some better way how to detect this?

Looks like you create the whole RequestLogger just to be able to tell if the URL is a Style or not.. which seems like an overkill. (+ There is no limit/eviction mechanism, so the memory consumption of RequestLogger will always only grow -- with each request.)

@jonathanlurie
Copy link
Collaborator Author

For the request logger, true, it would be more optimal to replace the JS Map with an LRU cache with a fixed max size.

As per the way to detect this, the reason is mainly driven by what happens here https://github.com/maplibre/maplibre-gl-js/blob/v4.7.0/src/ui/map.ts#L1845

I can try another approach that would be more local to a given map instance, but it's going to require a bit more logic about URL manipulation.

Will be creating a RD-324-2 branch for this. Putting this PRO on hold for the moment.

@jonathanlurie
Copy link
Collaborator Author

Alternative to this PR is here #124

@petrsloup
Copy link
Member

petrsloup commented Oct 14, 2024

Superseded by #124

@petrsloup petrsloup closed this Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants