Skip to content

Conversation

simonseyock
Copy link
Member

@simonseyock simonseyock commented Sep 28, 2019

Iterate through the comments two times. First collecting all identifiers and then replace them properly.

This fixes openlayers/openlayers#10049

@simonseyock simonseyock force-pushed the fix-local-types-conversion branch 3 times, most recently from fe33e65 to ffd1b31 Compare September 28, 2019 12:10
@simonseyock simonseyock force-pushed the fix-local-types-conversion branch from ffd1b31 to 206cd73 Compare September 28, 2019 12:21
@simonseyock
Copy link
Member Author

The second commit corrects an error where parts of types were matched.
i.e. PointCoordType was matched by the expression for Point

@simonseyock
Copy link
Member Author

This PR seems to have problems with @fires

@simonseyock
Copy link
Member Author

simonseyock commented Sep 29, 2019

I have it fixed mostly, but there is still the problem that many event links point to apidoc/module-ol_events_Event-Event.html which does not get generated.
So I am asking myself where it is defined, with a brief search I did not find any typedef. In the current documentation on openlayers.org it is present but is not findable via the search.

See https://openlayers.org/en/latest/apidoc/module-ol_events_Event-BaseEvent.html and https://openlayers.org/en/latest/apidoc/module-ol_events_Event-Event.html

As these two documents only differ in one detail (ExtentInteractionEvent instead of ExtentEvent), I searched for the term ExtentInteractionEvent and did not find it in the source code!

Could it be that the file is somehow a leftover from a previous generation? If that is the case, many types need to be changed from Event to BaseEvent.

This is not covered by typecheck because BaseEvent is a default export and always gets named BaseEvent.

I personally try to avoid default exports all together because it tends to get messy. But this is no change that could be done before v7.

So another way to solve this is in the jsdoc generation to first find out the real class names of every default export and afterwards change all the types accordingly. That means to iterate over the files two times.

@simonseyock simonseyock force-pushed the fix-local-types-conversion branch from e792eae to 4dafc0e Compare September 29, 2019 11:36
@simonseyock simonseyock force-pushed the fix-local-types-conversion branch from 4dafc0e to b46e436 Compare September 29, 2019 11:40
@ahocevar
Copy link
Member

ahocevar commented Sep 29, 2019

Could it be that the file is somehow a leftover from a previous generation? If that is the case, many types need to be changed from Event to BaseEvent.

Yes, this is the case. Our website generation process only overwrites files, but does not remove any. So it is better to debug this locally. There is only module-ol_events_Event-BaseEvent.html.

So another way to solve this is in the jsdoc generation to first find out the real class names of every default export and afterwards change all the types accordingly. That means to iterate over the files two times.

This would be the preferred way. But as things are currently related to TypeScript 3.7, a (fragile) workaround would be to name the imports accordingly in OpenLayers. See https://github.com/openlayers/openlayers/pull/10061/files#diff-079b05a573d5011f09c95c02d280ef16

@simonseyock
Copy link
Member Author

OK, if this does not occur elsewhere this would fix it. I will look into it.

@simonseyock simonseyock changed the title replace local identifiers after collecting all Fix type replacement issues Sep 29, 2019
@ahocevar ahocevar merged commit 18260ff into openlayers:master Sep 29, 2019
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.

Documentation glitch
2 participants