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

Don't throw on unsupported heatmap layer type instead show helpful debug info #1057

Merged

Conversation

orangemug
Copy link
Contributor

Previous to this change a style with a heatmap layer type would "throw" crashing the app. We now just console.debug with

[ol-mapbox-style] layers[1].type "heatmap" not supported

Reasons for console.debug instead of console warn/log, is that console.debug doesn't show up in the console by default. You have to enable verbose logging in the web inspector.

This also has the same behaviour as other unsupported styling rules, for example icon-pitch-alignment

@ahocevar
Copy link
Member

I'd prefer console.error over console.debug. It is easier to see when something goes wrong for those who have a developer console open, and others won't bother anyway.

@orangemug
Copy link
Contributor Author

The only problem with console.error is it's very noisy. So if you load a style that's has a heatmap, it is a valid style but it'll complain a lot. That means that if a person uses this library in their map (like I do) I get spammed with errors I have no way of currently resolving. For me this is on every style change as I update styles a lot when editing.

Also it's a little odd that layer types error when unsupported but unsupported properties don't.

Other options, worth considering

  1. Just don't log anything, which is in keeping with the rest of the libraries properties (later we could add a function to report/remove the unsupported properties, once Patched Mapbox GL Spec #10 is complete)
  2. Log via debug which isn't noisy by default, would require prod / non-prod builds (probs too complex for now)
  3. @orangemug just go ahead and use console.error(..)

Please pick an option (1, 2, or 3)

src/apply.js Outdated Show resolved Hide resolved
@ahocevar
Copy link
Member

For the case of things that are not supported by ol-mapbox-style, I think you're right that a debug message is appropriate.

@orangemug
Copy link
Contributor Author

orangemug commented Dec 16, 2023

I've made the changes, hopefully this should be good to merge now.

@ahocevar ahocevar merged commit bd5fe28 into openlayers:main Dec 16, 2023
1 check passed
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.

2 participants