Added geoJSON source getBounds method #5403
Conversation
Signed-off-by: Abel Vázquez Montoro <abel.vm@gmail.com>
|
There are problems with transpiling the code, and some nitpicking, but otherwise this looks good, thanks! |
Bumps [dependabot/fetch-metadata](https://github.com/dependabot/fetch-metadata) from 2.2.0 to 2.3.0. - [Release notes](https://github.com/dependabot/fetch-metadata/releases) - [Commits](dependabot/fetch-metadata@v2.2.0...v2.3.0) --- updated-dependencies: - dependency-name: dependabot/fetch-metadata dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [stylelint](https://github.com/stylelint/stylelint) from 16.13.2 to 16.14.0. - [Release notes](https://github.com/stylelint/stylelint/releases) - [Changelog](https://github.com/stylelint/stylelint/blob/main/CHANGELOG.md) - [Commits](stylelint/stylelint@16.13.2...16.14.0) --- updated-dependencies: - dependency-name: stylelint dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [@vitest/ui](https://github.com/vitest-dev/vitest/tree/HEAD/packages/ui) from 3.0.3 to 3.0.4. - [Release notes](https://github.com/vitest-dev/vitest/releases) - [Commits](https://github.com/vitest-dev/vitest/commits/v3.0.4/packages/ui) --- updated-dependencies: - dependency-name: "@vitest/ui" dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
|
Changes committed and changelog message added 🤓 |
AbelVM
changed the title
|
Ok, something is weird here. It looks like Any help is welcome |
|
GeoJson.Geojson is both feature collection and feature. You need to check which one you received and act accordingly. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5403 +/- ##
==========================================
- Coverage 91.94% 91.86% -0.09%
==========================================
Files 282 282
Lines 39039 39076 +37
Branches 6849 6849
==========================================
+ Hits 35896 35897 +1
- Misses 3016 3051 +35
- Partials 127 128 +1 ☔ View full report in Codecov by Sentry. |
|
Ok, it looks fine now |
|
Try adding a test to the browser.test.ts (integration tests) instead of unit test, it might be easier... |
|
Same results |
|
Can't help with a code I can't see, try checking it in so that I can take a look... |
|
Sorry, I tested it locally. Now its uploaded |
… multiple errors local
… multiple errors local
… multiple errors local
… multiple errors local
|
Ok, now everything related to this feature is working as expected 🎉 Coverage is low because there's no unit test as they're run as integration tests. I have no idea how to improve that, as |
| center: [10, 10], | ||
| zoom: 10 | ||
| }); | ||
| map.on('load', () => { |
There was a problem hiding this comment.
Use await once(...) to make less indentation and make sure the test waits from the map to load.
There was a problem hiding this comment.
It looks like await map.once('load'); doesn't work as expected.
-
.once()only works withMapLayerEventType, andloadis a MapEventType, so... not supported .on()works withMapEventType, but the listener is compulsory and returns a Subscription, not a Promise
I need to move back to map.on('load' =>{...})
(and we might need to change the behavior of once() to reflect the one of on())
There was a problem hiding this comment.
I'm not sure what you are talking about, the unit tests are filled with await once('load');, here's one for example:
There was a problem hiding this comment.
I'm just posting the errors the tests gave me (commit 6db23408202feea1a9967c083fb07a06b0a9b8db) in a human-readable way.
I've gone to check that weirdness in the documentation, and those in my message above are the links 🤷
There was a problem hiding this comment.
on amd once are different...
Using await on(...) shouldn't work as it's not designed to work this way.
Regarding the tests failures after the once change, the error message indicate that the expected object is "{}" which I find odd, regardless of the test failure. I would advise to try and run this code is a jsbin and see how it works, and after it works as expected, copy it to this file...
There was a problem hiding this comment.
getData is using messages to communicate with workers, so it you want to use that, you'll need to mock the workers communication since it's running in node environment and not in the browser.
When running things in the browser I expect it to just work, like it does in the examples we have in the docs, which uses this kind of await once code...
There was a problem hiding this comment.
Both bounds and testbounds are being used with their default values because await map.once() is not working as expected and... just doesn't wait 😅 So the flow reaches the assessment but the code to get the bounds has not run
The problem I found with the tests is that running them locally or even building sample code, behaves differently than within the contexts that Jenkins/GH Actions/Whatever uses. I get different errors, messages, and so on. That's why I'm sending too many commits, to getting actionable information from the online tests that I don't get locally
There was a problem hiding this comment.
This is not resolved.
Please try and use codespaces if you can't reproduce the issue locally...
There was a problem hiding this comment.
Any help is welcome, I wasn't able to make the integration tests work using await map.once().
In that very same file, the pattern is map.once('load', callback) (even when load event is not supported, as per the docs, by map.once)
| 'properties': {}, | ||
| 'geometry': { | ||
| 'type': 'LineString', | ||
| 'coordinates': [ |
There was a problem hiding this comment.
Can the geometry be simplified (2-3 points)?
There was a problem hiding this comment.
Sure, but I've just reused the data in the unit tests
There was a problem hiding this comment.
Then the data there probably needs simplification as well...
There was a problem hiding this comment.
I don't see it... Did you forget to push?
There was a problem hiding this comment.
| const bounds: LngLatBounds = new LngLatBounds(); | ||
| const data: GeoJSON.GeoJSON = await this.getData(); | ||
| let coordinates: number[]; | ||
| if (!!data) return bounds; |
There was a problem hiding this comment.
Not sure this is what I would expect as a return value...
There was a problem hiding this comment.
IMHO, if the source has no data, its bounding box should be empty
Resolves #5394