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

Throw an error when a used source is removed and not readded #5562

Merged
merged 4 commits into from
Nov 21, 2017

Conversation

lucaswoj
Copy link
Contributor

Currently, the below code does not throw an error. Instead it quietly fails to render mapbox-layer.

map.setStyle({
  'version': 8,
  'sources': {
    'mapbox-source': {...}
  },
  'layers': [{
    'id': 'mapbox-layer',
    'source': 'mapbox-source',
    'type': 'circle'
  }]
});

map.removeSource('mapbox-source');
// THROWS

This PR causes an error to be fired in the above case while still allowing the below case where a source is removed and then re-added before the next call to Style#update

map.removeSource('mapbox-source');
map.addSource('mapbox-source', {...});
// DOES NOT THROW

fixes #1711

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page

Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

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

In the aim of simplicity and consistency with native, I don't think we should support the "temporarily remove an in use source" case. If you attempt to remove an in use source, it should immediately produce an error.

@lucaswoj
Copy link
Contributor Author

@jfirebaugh I simplified per your feedback. I prefer this way but it may cause headaches for some users relying on the edge case. What do you think about using a warning & deprecation notice rather than an error?

@jfirebaugh
Copy link
Contributor

Signal the error via this.fire('error', ...) rather than an exception -- that's the intent for all style errors, and will automatically make the error behavior softer. It'll be a console error rather than stack unwinding.

Secondarily, I favor making the change now to return immediately in the error case (i.e. not removing the layer), but if you want to delay making that change for a release or two that's ok too. Let's leave the issue open if you do though.

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

Congrats on the first PR after a while!

@lucaswoj
Copy link
Contributor Author

lucaswoj commented Nov 1, 2017

✅ I made the suggested changes to the error handling flow. We'll should come back to this file and get rid of existing uses of throw with some kind of migration strategy for users who depend on catching those.

Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

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

💯

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.

Display an error message when a used source is removed and not re-added
3 participants