Skip to content

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

Open
jstaab wants to merge 5 commits intodanmarshall:masterfrom
jstaab:pr2
Open

Support for Non-Iterable Data, Data Accessor type inference, Extension props#232
jstaab wants to merge 5 commits intodanmarshall:masterfrom
jstaab:pr2

Conversation

@jstaab
Copy link
Copy Markdown
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
Copy Markdown
Owner

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

@jstaab
Copy link
Copy Markdown
Contributor Author

jstaab commented Feb 24, 2022

Sure thing. Just updated.

@danmarshall
Copy link
Copy Markdown
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.

jstaab added 5 commits April 12, 2022 10:01
…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