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

refactor!: move import progress from HTTP API to JS API #125

Merged
merged 2 commits into from
Feb 29, 2024
Merged

Conversation

EvanHahn
Copy link
Contributor

This is the next step in a larger refactor where we replace the HTTP API with a JS one.

This is the next step in [a larger refactor][0] where we replace the
HTTP API with a JS one.

[0]: #111

BREAKING CHANGE: `GET /imports/progress/:importId` replaced with
  `getImportProgress()`
package.json Show resolved Hide resolved
src/api/imports.ts Show resolved Hide resolved
src/lib/mbtiles_import_worker.ts Show resolved Hide resolved
src/lib/mbtiles_import_worker.ts Show resolved Hide resolved
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I converted this file to TypeScript because of the following problem:

  1. The external API now depends on PortMessage.
  2. Therefore, the external API needs to import PortMessage.
  3. dist/lib/mbtiles_import_worker.d.ts is the compiled version of src/lib/mb_tiles_import_worker.js and src/lib/mb_tiles_import_worker.d.ts` is lost.

I might have been able to solve this another way, such as duplicating (or moving?) the types elsewhere. However, I think this is a good solution given that the rest of the repo already uses TypeScript.

@@ -18,7 +18,7 @@ exports.createFastifyServer = createFastifyServer

/**
* @param {import('tape').Test} t
* @returns {import('../../src/map-server.ts')}
* @returns {ReturnType<import('../../src/map-server.ts')>}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not strictly related, but fixes these types.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think it should technically point to app.ts, which is where createMapServer() is imported from. What you have here returns the same thing though

@EvanHahn EvanHahn marked this pull request as ready for review February 27, 2024 23:33
Copy link
Member

@achou11 achou11 left a comment

Choose a reason for hiding this comment

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

great stuff! left a few comments but nothing noticeably blocking

package.json Show resolved Hide resolved
test/e2e/imports.test.js Outdated Show resolved Hide resolved
@@ -18,7 +18,7 @@ exports.createFastifyServer = createFastifyServer

/**
* @param {import('tape').Test} t
* @returns {import('../../src/map-server.ts')}
* @returns {ReturnType<import('../../src/map-server.ts')>}
Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think it should technically point to app.ts, which is where createMapServer() is imported from. What you have here returns the same thing though

test/e2e/tilesets-import.test.js Outdated Show resolved Hide resolved
@EvanHahn EvanHahn merged commit 399d38c into master Feb 29, 2024
3 checks passed
@EvanHahn EvanHahn deleted the remove-sse branch February 29, 2024 03:00
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.

None yet

2 participants