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

Fix type narrowing of image source types #13221

Closed
wants to merge 1 commit into from

Conversation

ezzatron
Copy link

This is a draft PR attempting to fix #13220. I don't think it's the best approach, but it might be useful for discussions.

The core problem of #13220 is that the new ImageSource type is serving double-duty as the basis for the CanvasSource and VideoSource types. This means that the type property is currently typed as string, which prevents type narrowing from working on the Source type.

The solution here does work, but having ImageSource be a generic type is probably not the nicest way to do it. Perhaps the ImageSource type should be separated from its implementation?

Launch Checklist

  • Make sure the PR title is descriptive and preferably reflects the change from the user's perspective.
  • Add additional detail and context in the PR description (with screenshots/videos if there are visual changes).
  • Manually test the debug page. (N/A: no behavioural changes)
  • Write tests for all new functionality and make sure the CI checks pass. (I wasn't able to because Vitest type tests don't seem to work in this project's test suite)
  • Document any changes to public APIs. (N/A: no behavioural changes)
  • Post benchmark scores if the change could affect performance. (N/A: no behavioural changes)
  • Tag @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes. (N/A)
  • Tag @mapbox/gl-native if this PR includes shader changes or needs a native port. (N/A)

@CLAassistant
Copy link

CLAassistant commented Jul 12, 2024

CLA assistant check
All committers have signed the CLA.

@stepankuzmin
Copy link
Contributor

Hi @ezzatron, thanks for opening PR for this! While I agree that making ImageSource generic is not the nicest way to achieve proper narrowing, I decided to do the same as you proposed in this PR.

I'm closing this PR since this will be available in the upcoming v3.5.2 #13230

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.

New Source types in v3.5 don't support type narrowing via the type property
3 participants