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

Openlayers types #1959

Merged
merged 18 commits into from
Feb 22, 2021
Merged

Openlayers types #1959

merged 18 commits into from
Feb 22, 2021

Conversation

simonseyock
Copy link
Member

FEATURE | BUGFIX

This PR introduces the @hanreev/ol-types package and fixes all openlayers typing errors.

@simonseyock
Copy link
Member Author

simonseyock commented Feb 1, 2021

One thing that should get checked, are the attributions. But this is unrelated to this PR, see #1962

I used the public function getUid instead of the private properties ol_uid.

I fixed the types of the timeLayerAware function. There is now an correctly typed Option.

For the type ArrayTwoOrMore I introduced a type guard isArrayTwoOrMore that let you easily verify that an array is indeed of the type ArrayTwoOrMore.

Another new type guard is isWmsLayer which verifies that a generic Layer is a BaseTileLayer|BaseImageLayer.

@simonseyock
Copy link
Member Author

I reverted the changes related to the selects.

Copy link
Member

@KaiVolland KaiVolland left a comment

Choose a reason for hiding this comment

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

Nice one. Ol types would be very cool if they are up to date.
Please have a look the import statements to have the Ol prefix.

Even if the changes are mostly about typing i think they introduce api changes, so we have to release a major version (which is not negative).

@simonseyock
Copy link
Member Author

One thing about actuality of the types: They should change only in details on minor version changes of openlayers. The hanreev types base for now on ol v6.4 but I do not think this is something we need to worry about as we are only using ol v6.5 for now.

On the next major version jump we would need to look at it again, but I am working on type improvements in the openlayers jsdoc types that will make the auto generated types usable in our projects. The hanreev types are way better, as they provide more details, for example they type the events that will be passed to the listeners.

@simonseyock simonseyock force-pushed the openlayers-types branch 2 times, most recently from 29bc382 to cac1860 Compare February 18, 2021 10:25
Copy link
Member

@ahennr ahennr left a comment

Choose a reason for hiding this comment

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

Nice work @simonseyock .

Copy link
Member

@annarieger annarieger left a comment

Choose a reason for hiding this comment

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

Nice one, thanks for this.

I've added some notes which you maybe want to address.

Copy link
Member

@dnlkoch dnlkoch left a comment

Choose a reason for hiding this comment

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

Very nice @simonseyock, I just addressed some minor issues regarding our current naming conventions and a note about moving code to separate packages.

Copy link
Member

@KaiVolland KaiVolland left a comment

Choose a reason for hiding this comment

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

Thanks for the productive discussion and your effort in this. Feel free to merge once the ci is happy.

@simonseyock simonseyock merged commit 17ef4d7 into master Feb 22, 2021
@simonseyock simonseyock deleted the openlayers-types branch February 22, 2021 13:32
@simonseyock simonseyock mentioned this pull request Mar 18, 2021
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.

5 participants