Support for Non-Iterable Data, Data Accessor type inference, Extension props#232
Open
jstaab wants to merge 5 commits intodanmarshall:masterfrom
Open
Support for Non-Iterable Data, Data Accessor type inference, Extension props#232jstaab wants to merge 5 commits intodanmarshall:masterfrom
jstaab wants to merge 5 commits intodanmarshall:masterfrom
Conversation
Owner
|
Thanks again. Would you mind looking at the merge conflict? Another (small) PR was just merged just now. |
Contributor
Author
|
Sure thing. Just updated. |
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 infernumberon 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
attributesto replaceaccessorprops. 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 forTileJsontyping inMVTLayer.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).