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

Support for Non-Iterable Data, Data Accessor type inference, Extension props #232

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jstaab
Copy link
Contributor

@jstaab jstaab commented Feb 24, 2022

This PR is my attempt to use type inference to drive everything downstream of the data prop. For a simple, concrete example, that means if you pass a [1, 2, 3] on data, then your accessors infer number on their datum parameter.

Some caveats: Apologies for the one mega-commit at the end. I've done the majority of this work last year, and eventually gave up on rebasing individual commits (as I found issues, trying to get the fix in the right commit). If need be, I think I could break it up again. I will admit to having not tested or even used every Layer with type coverage here, and every one of them is impacted by this PR. And I'm quite certain there could be entire concepts I've missed.

Following on, I've tried to flesh out usage of Non-Iterable data, the usage of attributes to replace accessor props. I have commits locally that remove the accessor props from the constructor when they're found in attributes, but haven't included that here.

The last major point is conditional props--ones which are only available when extensions are present. Most of these are layer-specific, but at least one I recall (BrushingExtension) applies to all Layers. I've added support for these.

Lots of tiny odds and ends fixes...too many to list all of them here. One notable addition would be exposing a state generic (S) for use when extending classes. Another is adding support for TileJson typing in MVTLayer.

I've added tests that cover a portion of the work here, certainly not all. But open to beefing up testing, or any other changes you think would be appropriate. Based on our discussion in #231 I've also included a version bump and an updated Readme. I do think a number of open issues could be resolved as a result of this PR.

Would also like to note two drawbacks of this approach: First is that when typing errors do occur, say, a missed required prop, they are now long and can be indiscernible unless you know what to look for. And second, type inference is all or nothing (there are longstanding TS issues on this topic), so you either must rely entirely on inference or be explicit about everything--typical use case here is wanting to supply additional props not codified in the Layer props. That's a case where inference will not work (and it's probably best to extend the Layer first).

@danmarshall
Copy link
Owner

Thanks again. Would you mind looking at the merge conflict? Another (small) PR was just merged just now.

@jstaab
Copy link
Contributor Author

jstaab commented Feb 24, 2022

Sure thing. Just updated.

@danmarshall
Copy link
Owner

Since this is such a large PR I'm going to let it bake here for a while in hopes that another few community members can try it out.

…erable, promise, or non-iterable with data-accessors.

adds a state Parameter S to Layer classes

adds the extension props

fixes tests

updates OrthoView and moves viewState to viewProps

adds extensions tests

adds inference tests

tests data sources

fixes tests
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