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

Fix #192, #232 and #233. Refactor some comments. Don't disable interactivity for when there's an ongoing routing request. Properly cancel prev. requests #235

Merged
merged 43 commits into from
Aug 2, 2024

Conversation

smellyshovel
Copy link
Collaborator

@smellyshovel smellyshovel commented Aug 1, 2024

There are many things to this commit.

First of all, it fixes #232 by listening for map's idling after re-drawing necessary features on onDrugUp and onClick events, and when the map begins to idle, it calls the onMove method with the same coordinates the click/drug-up happened at. This could be considered an artifitial mouse movement which was previously required to be performed manually before the map could register a new mousemove event and understand that the cursor is still hovering a waypoint.

Next, there's a fix for #233. There are 2 components to the fix. First, the onDrugUp method was asynchronois and was awaiting the fetchDirections call to finish, and only after that (regardless of whether it finished successfully or not) it detached the drag-specific event listeners. This led to long-taking requests creating wierd interaction-artifacts. Now, instead of being async. and actually waiting for the request to finish, the method only initiates it and proceeds straight away to disabling the (already-ended) drag-related event listeners. This created an issue with the try-catch block not being able to detect errors for requests anymore, but this is solved by moving from try-catch to Promise's catch clause.

The second component of the fix is to simplify the interaction handling for ongoing requests. Previously, we tried to control the flow of enabling/disabling the interaction for the plugin while there were requests being performed, but with this PR I decided not to touch it at all. So, starting from now it's possible to keep interacting with the features of the plugin even while there's a request to load routes from the prev. interaction. And an important change here is that it's not possible anymore to create any confusion between what's requested and what's displayed because any previous request is automatically discarded as soon as there's a new one. This is done by removing the abortController de-initialization after requests, as de-initializing it, because of race-conditions, sometimes de-initialized not the old one, but the new one. This is not the issue anymore.

Thanks to that, as a side-effect, this also fixed #192 as interactivity doesn't have to do anything anymore with fetching the directions.


Additionally, there are some small improvements like:

  1. Refactored comments (some of them which catched my eye)
  2. We don't rely anymore anywhere on the private _interactive field and instead only treat the public interactive flag as the only source of truth.

dependabot bot and others added 30 commits February 1, 2024 06:33
Bumps [svelte-spa-router](https://github.com/ItalyPaleAle/svelte-spa-router) from 3.3.0 to 4.0.1.
- [Release notes](https://github.com/ItalyPaleAle/svelte-spa-router/releases)
- [Changelog](https://github.com/ItalyPaleAle/svelte-spa-router/blob/main/CHANGELOG.md)
- [Commits](ItalyPaleAle/svelte-spa-router@v3.3.0...v4.0.1)

---
updated-dependencies:
- dependency-name: svelte-spa-router
  dependency-type: direct:development
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [eslint](https://github.com/eslint/eslint) from 8.56.0 to 8.57.0.
- [Release notes](https://github.com/eslint/eslint/releases)
- [Changelog](https://github.com/eslint/eslint/blob/main/CHANGELOG.md)
- [Commits](eslint/eslint@v8.56.0...v8.57.0)

---
updated-dependencies:
- dependency-name: eslint
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [nanoid](https://github.com/ai/nanoid) from 5.0.4 to 5.0.6.
- [Release notes](https://github.com/ai/nanoid/releases)
- [Changelog](https://github.com/ai/nanoid/blob/main/CHANGELOG.md)
- [Commits](ai/nanoid@5.0.4...5.0.6)

---
updated-dependencies:
- dependency-name: nanoid
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [postcss-load-config](https://github.com/postcss/postcss-load-config) from 4.0.2 to 5.0.3.
- [Release notes](https://github.com/postcss/postcss-load-config/releases)
- [Changelog](https://github.com/postcss/postcss-load-config/blob/main/CHANGELOG.md)
- [Commits](postcss/postcss-load-config@v4.0.2...v5.0.3)

---
updated-dependencies:
- dependency-name: postcss-load-config
  dependency-type: direct:development
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [prettier-plugin-svelte](https://github.com/sveltejs/prettier-plugin-svelte) from 3.1.2 to 3.2.2.
- [Changelog](https://github.com/sveltejs/prettier-plugin-svelte/blob/master/CHANGELOG.md)
- [Commits](sveltejs/prettier-plugin-svelte@v3.1.2...v3.2.2)

---
updated-dependencies:
- dependency-name: prettier-plugin-svelte
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [lint-staged](https://github.com/okonet/lint-staged) from 15.2.0 to 15.2.2.
- [Release notes](https://github.com/okonet/lint-staged/releases)
- [Changelog](https://github.com/lint-staged/lint-staged/blob/master/CHANGELOG.md)
- [Commits](lint-staged/lint-staged@v15.2.0...v15.2.2)

---
updated-dependencies:
- dependency-name: lint-staged
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [svelte](https://github.com/sveltejs/svelte/tree/HEAD/packages/svelte) from 4.2.8 to 4.2.12.
- [Release notes](https://github.com/sveltejs/svelte/releases)
- [Changelog](https://github.com/sveltejs/svelte/blob/[email protected]/packages/svelte/CHANGELOG.md)
- [Commits](https://github.com/sveltejs/svelte/commits/[email protected]/packages/svelte)

---
updated-dependencies:
- dependency-name: svelte
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [prettier](https://github.com/prettier/prettier) from 3.1.1 to 3.2.5.
- [Release notes](https://github.com/prettier/prettier/releases)
- [Changelog](https://github.com/prettier/prettier/blob/main/CHANGELOG.md)
- [Commits](prettier/prettier@3.1.1...3.2.5)

---
updated-dependencies:
- dependency-name: prettier
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [husky](https://github.com/typicode/husky) from 8.0.3 to 9.0.11.
- [Release notes](https://github.com/typicode/husky/releases)
- [Commits](typicode/husky@v8.0.3...v9.0.11)

---
updated-dependencies:
- dependency-name: husky
  dependency-type: direct:development
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [svelte-check](https://github.com/sveltejs/language-tools) from 3.6.2 to 3.6.8.
- [Release notes](https://github.com/sveltejs/language-tools/releases)
- [Commits](sveltejs/language-tools@svelte-check-3.6.2...svelte-check-3.6.8)

---
updated-dependencies:
- dependency-name: svelte-check
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [postcss](https://github.com/postcss/postcss) from 8.4.33 to 8.4.38.
- [Release notes](https://github.com/postcss/postcss/releases)
- [Changelog](https://github.com/postcss/postcss/blob/main/CHANGELOG.md)
- [Commits](postcss/postcss@8.4.33...8.4.38)

---
updated-dependencies:
- dependency-name: postcss
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [@tsconfig/svelte](https://github.com/tsconfig/bases/tree/HEAD/bases) from 5.0.2 to 5.0.4.
- [Commits](https://github.com/tsconfig/bases/commits/HEAD/bases)

---
updated-dependencies:
- dependency-name: "@tsconfig/svelte"
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [@types/lodash](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/lodash) from 4.14.202 to 4.17.0.
- [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases)
- [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/lodash)

---
updated-dependencies:
- dependency-name: "@types/lodash"
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [autoprefixer](https://github.com/postcss/autoprefixer) from 10.4.16 to 10.4.19.
- [Release notes](https://github.com/postcss/autoprefixer/releases)
- [Changelog](https://github.com/postcss/autoprefixer/blob/main/CHANGELOG.md)
- [Commits](postcss/autoprefixer@10.4.16...10.4.19)

---
updated-dependencies:
- dependency-name: autoprefixer
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
…ging/autoprefixer-10.4.19

chore(deps-dev): bump autoprefixer from 10.4.16 to 10.4.19
…ging/types/lodash-4.17.0

chore(deps-dev): bump @types/lodash from 4.14.202 to 4.17.0
…ging/tsconfig/svelte-5.0.4

chore(deps-dev): bump @tsconfig/svelte from 5.0.2 to 5.0.4
…ging/postcss-8.4.38

chore(deps-dev): bump postcss from 8.4.33 to 8.4.38
Bumps [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/parser) from 6.18.1 to 7.7.0.
- [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases)
- [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/parser/CHANGELOG.md)
- [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v7.7.0/packages/parser)

---
updated-dependencies:
- dependency-name: "@typescript-eslint/parser"
  dependency-type: direct:development
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [tailwindcss](https://github.com/tailwindlabs/tailwindcss) from 3.4.1 to 3.4.3.
- [Release notes](https://github.com/tailwindlabs/tailwindcss/releases)
- [Changelog](https://github.com/tailwindlabs/tailwindcss/blob/v3.4.3/CHANGELOG.md)
- [Commits](tailwindlabs/tailwindcss@v3.4.1...v3.4.3)

---
updated-dependencies:
- dependency-name: tailwindcss
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
…ging/typescript-eslint/parser-7.7.0

chore(deps-dev): bump @typescript-eslint/parser from 6.18.1 to 7.7.0
…ging/tailwindcss-3.4.3

chore(deps-dev): bump tailwindcss from 3.4.1 to 3.4.3
…ging/svelte-check-3.6.8

chore(deps-dev): bump svelte-check from 3.6.2 to 3.6.8
…ging/husky-9.0.11

chore(deps-dev): bump husky from 8.0.3 to 9.0.11
…ging/prettier-plugin-svelte-3.2.2

chore(deps-dev): bump prettier-plugin-svelte from 3.1.2 to 3.2.2
…aging/postcss-load-config-5.0.3' into staging

# Conflicts:
#	package-lock.json
#	package.json
…aging/nanoid-5.0.6' into staging

# Conflicts:
#	package-lock.json
…aging/lint-staged-15.2.2' into staging

# Conflicts:
#	package-lock.json
#	package.json
…aging/eslint-8.57.0' into staging

# Conflicts:
#	package.json
…aging/prettier-3.2.5' into staging

# Conflicts:
#	package.json
@smellyshovel smellyshovel changed the title TODO Fix #192, #232 and #233. Refactor some comments. Don't disable interactivity for when there's on ongoing routing request. Properly cancel prev. requests Aug 1, 2024
@smellyshovel smellyshovel marked this pull request as ready for review August 1, 2024 16:18
@smellyshovel smellyshovel added bug Something isn't working semver:patch For non-breaking PR's that don't introduce new features labels Aug 1, 2024
@smellyshovel
Copy link
Collaborator Author

@joleeee might be interesting for you as well. A review is welcome.

@smellyshovel smellyshovel changed the title Fix #192, #232 and #233. Refactor some comments. Don't disable interactivity for when there's on ongoing routing request. Properly cancel prev. requests Fix #192, #232 and #233. Refactor some comments. Don't disable interactivity for when there's an ongoing routing request. Properly cancel prev. requests Aug 1, 2024
@smellyshovel
Copy link
Collaborator Author

I'm also not sure TBH whether it's a patch or a minor release. On one hand, there are no new features and breaking changes, but on the other hand, if the way we work with the interaction while there's an ongoing request has changed, doesn't it mean that the behavior of the plugin is now different and thus it's a minor release? Or even a breaking change?

Marking it as a patch for now though, as I believe that it's a pretty much inner thing and no one will ever notice the change.

@joleeee
Copy link

joleeee commented Aug 1, 2024

Nice! It seems way more predictable now. Still having an issue which is perhaps not really encapsulated by #232 and #233. If the route updates while dragging, the waypoint get frozen and is "forgotten". As in, the route does not update from where the waypoint now is, it just freezes mid motion.

rec.mp4

@joleeee
Copy link

joleeee commented Aug 1, 2024

I feel like it's a bit big for just a patch, but at the same time, I feel like this just makes the experience so much better. I can't see why anyone would want to not upgrade.

@smellyshovel
Copy link
Collaborator Author

smellyshovel commented Aug 1, 2024

Nice! It seems way more predictable now. Still having an issue which is perhaps not really encapsulated by #232 and #233. If the route updates while dragging, the waypoint get frozen and is "forgotten". As in, the route does not update from where the waypoint now is, it just freezes mid motion.

rec.mp4

Definitely another effort. Feel free to open a ticket for this one.

UPD. I think @Miguel-Sanches worked on live updates. And I remember it being a huge pain in you know where, but mostly because of these setTimeouts and interactivity enabling/disabling. Am I right? What if we now make it simpler? Just on every mousemove we can call the fetchDirections and they would automatically cancel with each follow-up request. This wasn't possible before as the canceling wasn't working properly. Or am I missing something?

@Miguel-Sanches
Copy link
Collaborator

UPD. I think @Miguel-Sanches worked on live updates. And I remember it being a huge pain in you know where, but mostly because of these setTimeouts and interactivity enabling/disabling. Am I right? What if we now make it simpler? Just on every mousemove we can call the fetchDirections and they would automatically cancel with each follow-up request. This wasn't possible before as the canceling wasn't working properly. Or am I missing something?

Yeap, you're right. we tried doing it that way back then but it would fail to work once every 3/4 times due to canceling not working properly.

But we can try to make it simpler now. I think the effort wouldn't be in vain

@smellyshovel smellyshovel merged commit 0a12345 into maplibre:main Aug 2, 2024
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working semver:patch For non-breaking PR's that don't introduce new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Does not register mouse is hovering Disabling interactivity is ignored if disabled while fetching directions
3 participants