Combine `ResourceType` interface and const into a single enum #2103

smellyshovel · 2023-01-27T08:38:33Z

Part 2 of series of refactoring PRs (splitted up #2038).

Combines interface and const ResourceType into a single const enum enity called ResourceType and moves it from ajax.ts to request_manager.ts.

Reasons:

No need to split an enum into interface and const and then to create an artificial type from this interface
ajax.ts is not responsible for resource types nor they are used in there. request_manager.ts is using them and thus should be responsible for its definition

Move it from ajax.ts to request_manager.ts

smellyshovel · 2023-01-27T08:50:55Z

@HarelM ready for review.

src/util/request_manager.ts

HarelM

The only comment I have is string vs enum. If we can keep it as string I would prefer it instead of breaking this very much used API. I know it's version 3 and we can break, but if possible, I would like to avoid breaking the transform function.

drwestco · 2023-01-27T15:06:54Z

Strongly disagree with adding the MapLibre prefix to types. It just adds noise.

HarelM · 2023-01-27T15:25:36Z

I don't have hard feelings regarding the prefix. I can live with it and without it.

drwestco · 2023-01-27T15:46:48Z

It's just unnecessary code churn. Unless there's another ResourceType we're trying to avoid collisions with?

smellyshovel · 2023-01-28T08:15:43Z

It's just unnecessary code churn. Unless there's another ResourceType we're trying to avoid collisions with?

Once again, that's done for the sakes of consistency with other custom types (this will be implemented with further PRs). Namely, there's a built-in type called Request. And we'll need a custom type to represent MapLibre requests. For that purpose there'll be MapLibreRequest type. And MapLibreResponse. By analogy, this enum should be called MapLibreResourceType.

HarelM

LGTM, @drwestco any other comments?

drwestco · 2023-01-28T16:23:25Z

It's a slippery slope, and an unnecessary rename in this case. Even if you are forced to rename the existing Request type to avoid conflicts with the Fetch API (and I'd pick a different name than adding MapLibre as a prefix in that case), there's no reason ResourceType needs to change to match.

Take a look at the exported (and publicly documented, sorta) function signature:

export type RequestTransformFunction = (url: string, resourceType?: MapLibreResourceType) => RequestParameters;

Is this also going to become MapLibreRequestTransformFunction, returning a MapLibreRequestParameters object?

src/util/request_manager.ts

smellyshovel · 2023-01-30T09:23:27Z

@drwestco I thought about it all and both are actually good points. Let's not use MapLibre prefixes. When it will come to the Request type, I'll try to come up with some different prefix to avoid confusion with the built-in Request, but that's a story for another PR.

Regarding the const enum - I agree as well. TBH, did even not know about the difference between enum and const enum. And, apparently, there's indeed no value in having the type available in runtime. Thanks for that tip!

@HarelM Please, re-check.

HarelM · 2023-01-30T09:29:01Z

Wow, this makes this PR super simple.
No risk here. merging.

* Remove unused tile request cache (maplibre#2101) * Remove unused tile request cache * Remove the public method to clear the storage * Get rid of tile request cache's usages * merge 2101 * Combine `ResourceType` interface and const into a single enum (maplibre#2103) * Combine ResourceType interface and const into a single enum Move it from ajax.ts to request_manager.ts * Re-run render tests * Assign strting values to the enum * Revert changes to tests * Fix typo * Don't use MapLibre prefix; use const enum * Fix "npm run test-build" on Windows (maplibre#2106) * wip * fixed path * rename var * remove timeout * add type and use namespace * test passed * refactored and cleaned * min test * all working * restore min test --------- Co-authored-by: Matthew Mamonov <g.smellyshovel@gmail.com>

* Remove unused tile request cache (maplibre#2101) * Remove unused tile request cache * Remove the public method to clear the storage * Get rid of tile request cache's usages * merge 2101 * Combine `ResourceType` interface and const into a single enum (maplibre#2103) * Combine ResourceType interface and const into a single enum Move it from ajax.ts to request_manager.ts * Re-run render tests * Assign strting values to the enum * Revert changes to tests * Fix typo * Don't use MapLibre prefix; use const enum * Fix "npm run test-build" on Windows (maplibre#2106) * wip * fixed path * rename var * remove timeout * add type and use namespace * test passed * refactored and cleaned * min test * all working * restore min test * clean up the cancel function --------- Co-authored-by: Matthew Mamonov <g.smellyshovel@gmail.com>

…erformance (#2097) * Throttle the image request queue while the map is moving to improve performance The image request queue can cause frame rate glitches while the map is moving. To mitigate this, allow the map renderer to throttle the amount of requests processed by the image request queue until the map stops moving again. * Add AJAX unit test. * Fix cancelRequest function * lint fixes * Fix typo in comment. * Make amount of throttling be configurable. * Rename MAX_PARALLEL_IMAGE_REQUESTS_WHILE_THROTTLED to MAX_PARALLEL_IMAGE_REQUESTS_PER_FRAME_WHILE_THROTTLED * Add getter/setter maxParallelImageRequestsPerFrameWhileThrottled() * Fix lint issue. * Fix math error when computing maxImageRequests. * Add changelog entry * Move image request queue functions out of ajax.ts and into new image_request_queue.ts. * Move image request queue code back into ajax.ts to minimize diffs. * Reorganize image request queue code into a class (ImageRequestQueue). * Add jsdoc comments * Modify ImageRequestQueue class to call itself directly rather than via the pure function wrappers. * Remove one instance of `theQueue` syntax that wasn't necessary. * Fix unit test failure. * Update changelog with PR 2097 * merge 2101 * PR updates (#3) * Remove unused tile request cache (#2101) * Remove unused tile request cache * Remove the public method to clear the storage * Get rid of tile request cache's usages * merge 2101 * Combine `ResourceType` interface and const into a single enum (#2103) * Combine ResourceType interface and const into a single enum Move it from ajax.ts to request_manager.ts * Re-run render tests * Assign strting values to the enum * Revert changes to tests * Fix typo * Don't use MapLibre prefix; use const enum * Fix "npm run test-build" on Windows (#2106) * wip * fixed path * rename var * remove timeout * add type and use namespace * test passed * refactored and cleaned * min test * all working * restore min test * clean up the cancel function --------- Co-authored-by: Matthew Mamonov <g.smellyshovel@gmail.com> * lint * update to 754099 * PR feedback * doc --------- Co-authored-by: Adam Szofran <adam.szofran@microsoft.com> Co-authored-by: Matthew Mamonov <g.smellyshovel@gmail.com>

smellyshovel added 2 commits January 27, 2023 15:33

Combine ResourceType interface and const into a single enum

9e6584b

Move it from ajax.ts to request_manager.ts

Re-run render tests

3d09fa6

HarelM reviewed Jan 27, 2023

View reviewed changes

src/util/request_manager.ts Outdated Show resolved Hide resolved

HarelM reviewed Jan 27, 2023

View reviewed changes

smellyshovel added 2 commits January 28, 2023 15:10

Assign strting values to the enum

501d7c2

Revert changes to tests

718394f

Fix typo

40a721f

HarelM approved these changes Jan 28, 2023

View reviewed changes

drwestco reviewed Jan 28, 2023

View reviewed changes

src/util/request_manager.ts Outdated Show resolved Hide resolved

Don't use MapLibre prefix; use const enum

6aaea16

HarelM approved these changes Jan 30, 2023

View reviewed changes

HarelM merged commit e0d9c87 into maplibre:main Jan 30, 2023

smellyshovel deleted the refact/resource-type branch January 30, 2023 10:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Combine `ResourceType` interface and const into a single enum #2103

Combine `ResourceType` interface and const into a single enum #2103

smellyshovel commented Jan 27, 2023 •

edited

Loading

smellyshovel commented Jan 27, 2023

HarelM left a comment •

edited

Loading

drwestco commented Jan 27, 2023

HarelM commented Jan 27, 2023

drwestco commented Jan 27, 2023

smellyshovel commented Jan 28, 2023 •

edited

Loading

HarelM left a comment

drwestco commented Jan 28, 2023

smellyshovel commented Jan 30, 2023

HarelM commented Jan 30, 2023

Combine ResourceType interface and const into a single enum #2103

Combine ResourceType interface and const into a single enum #2103

Conversation

smellyshovel commented Jan 27, 2023 • edited Loading

smellyshovel commented Jan 27, 2023

HarelM left a comment • edited Loading

Choose a reason for hiding this comment

drwestco commented Jan 27, 2023

HarelM commented Jan 27, 2023

drwestco commented Jan 27, 2023

smellyshovel commented Jan 28, 2023 • edited Loading

HarelM left a comment

Choose a reason for hiding this comment

drwestco commented Jan 28, 2023

smellyshovel commented Jan 30, 2023

HarelM commented Jan 30, 2023

Combine `ResourceType` interface and const into a single enum #2103

Combine `ResourceType` interface and const into a single enum #2103

smellyshovel commented Jan 27, 2023 •

edited

Loading

HarelM left a comment •

edited

Loading

smellyshovel commented Jan 28, 2023 •

edited

Loading