refactor: Specify common types TDE-1030 #849

l0b0 · 2024-02-01T03:50:35Z

Motivation

Strengthen type limitations to make it easier to understand where a "string" is actually a URL and where it's really a local path.

Checklist

Refactor; none of the checklist is applicable.

src/utils/types.ts

src/commands/basemaps-mapsheet/create-mapsheet.ts

src/commands/copy/copy.ts

blacha · 2024-02-06T23:02:44Z

src/commands/basemaps-mapsheet/create-mapsheet.ts

- const obj = await fsa.read(config);
- if (config.endsWith('.gz') || isGzip(obj)) {
+async function readConfig(config: URL): Promise<ConfigBundled> {
+ const obj = await fsa.read(config.href);


its not safe to use .href here. If its a file url it has to be converted with path.fileURLToPath.

does fsa.read not accept a URL? would imply fsa is out of date?

its not safe to use .href here. If its a file url it has to be converted with path.fileURLToPath.

I'd rather upgrade fsa, but:

does fsa.read not accept a URL? would imply fsa is out of date?

Yep; that PR needs a bunch of fixes, though.

src/commands/create-manifest/create-manifest.ts

blacha · 2024-02-06T23:10:34Z

src/commands/create-manifest/create-manifest.ts

 if (baseFile) {
- target = fsa.joinAll(targetPath, transformFunc ? transformFunc(baseFile) : baseFile);
+ target = new URL(fsa.joinAll(targetUrl.href, transformFunc ? transformFunc(baseFile) : baseFile));


joinAll does a very similar job to new URL(input, base) in most use cases it should be a direct swap.

I believe the one caveat is joinAll("foo.com/bar", "baz") would make "foo.com/bar/baz" where as new URL("baz", "foo.com/bar") would make "foo.com/baz"

Indeed, new URL('sub-path', 's3://bucket/path').href === "s3://bucket/sub-path" . But I don't know what to do about that info. I would expect most of our base URLs to be arbitrary non-root ones, in which case new URL(input, base) would not do what we want. Which means joinAll is the safer alternative. Or are you saying the base URLs will always end with a slash, so that both solutions do the same thing?

We also have a normaliseHref in src/commands/stac-validate/stac.validate.ts and tryParseUrl in src/commands/common.ts which should probably be removed.

src/utils/action.storage.ts

Bumps the chunkd group with 3 updates: [@chunkd/fs](https://github.com/blacha/chunkd/tree/HEAD/packages/fs), [@cogeotiff/core](https://github.com/blacha/cogeotiff/tree/HEAD/packages/core) and [@chunkd/source-memory](https://github.com/blacha/chunkd/tree/HEAD/packages/source-memory). Updates `@chunkd/fs` from 10.0.9 to 11.2.0 - [Release notes](https://github.com/blacha/chunkd/releases) - [Changelog](https://github.com/blacha/chunkd/blob/master/packages/fs/CHANGELOG.md) - [Commits](https://github.com/blacha/chunkd/commits/fs-v11.2.0/packages/fs) Updates `@cogeotiff/core` from 8.1.1 to 9.0.3 - [Release notes](https://github.com/blacha/cogeotiff/releases) - [Changelog](https://github.com/blacha/cogeotiff/blob/master/packages/core/CHANGELOG.md) - [Commits](https://github.com/blacha/cogeotiff/commits/cli-v9.0.3/packages/core) Updates `@chunkd/source-memory` from 10.1.0 to 11.0.2 - [Release notes](https://github.com/blacha/chunkd/releases) - [Changelog](https://github.com/blacha/chunkd/blob/master/packages/source-memory/CHANGELOG.md) - [Commits](https://github.com/blacha/chunkd/commits/fs-v11.0.2/packages/source-memory) --- updated-dependencies: - dependency-name: "@chunkd/fs" dependency-type: direct:production update-type: version-update:semver-major dependency-group: chunkd - dependency-name: "@cogeotiff/core" dependency-type: direct:production update-type: version-update:semver-major dependency-group: chunkd - dependency-name: "@chunkd/source-memory" dependency-type: direct:development update-type: version-update:semver-major dependency-group: chunkd ... Signed-off-by: dependabot[bot] <support@github.com>

src/commands/basemaps-mapsheet/create-mapsheet.ts

blacha · 2024-02-11T23:58:02Z

src/commands/copy/copy-rpc.ts

@@ -6,7 +6,7 @@ export interface CopyContractArgs {
 /** Copy ID for tracing */
 id: string;
 /** List of files that need to be copied */
- manifest: { source: string; target: string }[];
+ manifest: { source: URL; target: URL }[];


can worker threads actually pass URLS? I thought everything was meant to be a POJO for passing around?

I assume you mean either JSON or JS object, not "Plain old Java object". I have no idea how WorkerRpc works; I guess since you ask we don't have tests for this message passing?

worker_threads are a mechanisim of using small scripts to add more threads to do processing, they are limited to posting messages to/from the thread using a rpc type mechanisim, WorkerRpc wraps the posting so it is somewhat typesafe.

I am not sure we can pass a URL between threads as its not really a easily searliazable thing, just like you cant pass a date but you can pass a ISO string.

src/commands/create-manifest/create-manifest.ts

src/commands/stac-validate/stac.validate.ts

src/utils/action.storage.ts

Based on <https://en.wikipedia.org/wiki/File_URI_scheme#Number_of_slash_characters>.

Should not have been commented.

`new URL` can take URL as the second argument, so we don't need to do the `.href` lookup.

src/commands/basemaps-mapsheet/create-mapsheet.ts

They might not be able to deal with an `URL`.

Co-authored-by: Blayne Chard <bchard@linz.govt.nz>

blacha

These issues could have prevented the commands from running, it is slightly concerning that they are this deep in the pull request chain and I have likely missed other spots where things are broken.

Have you run any of the commands before making this PR?

blacha · 2024-02-18T20:24:24Z

src/commands/basemaps-mapsheet/create-mapsheet.ts

 logger.info({ config }, 'MapSheet:LoadConfig');
 const configJson = await readConfig(config);
 const mem = ConfigProviderMemory.fromJson(configJson);

 const rest = fgb.deserialize(buf) as FeatureCollection;
- const featuresWritePromise = fsa.write('features.json', JSON.stringify(rest));
+ const featuresWritePromise = fsa.write(new URL(fsa.toUrl('./features.json').href), JSON.stringify(rest));


Suggested change

const featuresWritePromise = fsa.write(new URL(fsa.toUrl('./features.json').href), JSON.stringify(rest));

const featuresWritePromise = fsa.write(fsa.toUrl('./features.json'), JSON.stringify(rest));

I was using the wrong interpreter; please ignore the deleted comment.

src/commands/stac-validate/stac.validate.ts

l0b0 · 2024-02-19T00:16:07Z

These issues could have prevented the commands from running, it is slightly concerning that they are this deep in the pull request chain and I have likely missed other spots where things are broken.

Have you run any of the commands before making this PR?

No, because making heaps of manual smoke tests for each refactor is tedious, error-prone, and slow. If it's worth testing we should have automated tests for it.

refactor: Specify common branded types

25f890c

l0b0 requested review from a team as code owners February 1, 2024 03:50

l0b0 requested review from blacha, amfage, MDavidson17 and paulfouquet February 1, 2024 03:50

l0b0 commented Feb 1, 2024

View reviewed changes

src/utils/types.ts Outdated Show resolved Hide resolved

docs: Explain branded types

243ceeb

blacha reviewed Feb 1, 2024

View reviewed changes

src/commands/basemaps-mapsheet/create-mapsheet.ts Outdated Show resolved Hide resolved

blacha reviewed Feb 1, 2024

View reviewed changes

src/commands/copy/copy.ts Outdated Show resolved Hide resolved

l0b0 added 2 commits February 2, 2024 14:02

chore: Remove unused special case for JSON input

a119ba7

refactor: Use built-in URL type

907f18a

l0b0 requested a review from blacha February 2, 2024 01:48

l0b0 changed the title ~~refactor: Specify common branded types~~ refactor: Specify common types Feb 2, 2024

Merge remote-tracking branch 'origin/master' into refactor/types

8f8c146

blacha reviewed Feb 6, 2024

View reviewed changes

src/commands/create-manifest/create-manifest.ts Outdated Show resolved Hide resolved

blacha reviewed Feb 6, 2024

View reviewed changes

src/utils/action.storage.ts Outdated Show resolved Hide resolved

l0b0 added 3 commits February 7, 2024 13:13

fix: Revert wrong use of base64url encoding

8d716c3

refactor: Re-use UrlParser

3fe4a21

fix: Handle local path in $ACTION_PATH

9ca2b9e

l0b0 requested a review from blacha February 7, 2024 00:34

l0b0 and others added 6 commits February 7, 2024 13:37

refactor: Use UrlParser instead of tryParseUrl

aeeb40a

refactor: Use fsa.join instead of normaliseHref

48f5b2d

fix: Use URL for STAC URL concatenation

cf7aad3

fix: Work with latest chunkd update

73e08c1

refactor: fixup dependencies

e0f9241

refactor: Join imports

1f36e73

blacha reviewed Feb 11, 2024

View reviewed changes

src/commands/basemaps-mapsheet/create-mapsheet.ts Outdated Show resolved Hide resolved

blacha reviewed Feb 11, 2024

View reviewed changes

src/commands/create-manifest/create-manifest.ts Outdated Show resolved Hide resolved

blacha reviewed Feb 12, 2024

View reviewed changes

src/commands/stac-validate/stac.validate.ts Outdated Show resolved Hide resolved

blacha reviewed Feb 12, 2024

View reviewed changes

src/utils/action.storage.ts Outdated Show resolved Hide resolved

l0b0 added 6 commits February 12, 2024 13:20

fix: Use absolute path for file URL

3b5eb17

Based on <https://en.wikipedia.org/wiki/File_URI_scheme#Number_of_slash_characters>.

chore: Uncomment test

dcd9868

Should not have been commented.

chore: Remove redundant lookup of URL.href

f09b7d3

`new URL` can take URL as the second argument, so we don't need to do the `.href` lookup.

refactor: Use existing fsa.toUrl

5306cc8

refactor: Simplify by using synchronous URL parser

273235e

Merge remote-tracking branch 'origin/master' into refactor/types

cc9ddb1

l0b0 requested review from blacha and paulfouquet February 12, 2024 02:20

blacha reviewed Feb 13, 2024

View reviewed changes

src/commands/basemaps-mapsheet/create-mapsheet.ts Outdated Show resolved Hide resolved

l0b0 and others added 4 commits February 14, 2024 09:05

fix: Pass plain strings to workers

87c913f

They might not be able to deal with an `URL`.

Update src/commands/basemaps-mapsheet/create-mapsheet.ts

894218c

Co-authored-by: Blayne Chard <bchard@linz.govt.nz>

refactor: Let URL constructor deal with relative paths

1a26e94

fix: Syntax and type

767edc0

l0b0 requested a review from blacha February 13, 2024 20:15

blacha requested changes Feb 18, 2024

View reviewed changes

l0b0 requested a review from blacha February 19, 2024 00:16

l0b0 added 4 commits February 20, 2024 09:22

Merge remote-tracking branch 'origin/master' into refactor/types

fc92748

refactor: Simplify URL generation

ffd1d02

refactor: Create URL with library method

8191600

Merge remote-tracking branch 'origin/master' into refactor/types

79737fc

l0b0 closed this Feb 23, 2024

auto-merge was automatically disabled February 23, 2024 01:52
Pull request was closed

l0b0 deleted the refactor/types branch March 17, 2024 20:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Specify common types TDE-1030 #849

refactor: Specify common types TDE-1030 #849

l0b0 commented Feb 1, 2024 •

edited

Loading

blacha Feb 6, 2024

l0b0 Feb 7, 2024 •

edited

Loading

blacha Feb 6, 2024

l0b0 Feb 7, 2024 •

edited

Loading

blacha Feb 11, 2024

l0b0 Feb 12, 2024

blacha Feb 13, 2024

blacha left a comment •

edited

Loading

blacha Feb 18, 2024

l0b0 Feb 19, 2024

l0b0 commented Feb 19, 2024

	const featuresWritePromise = fsa.write(new URL(fsa.toUrl('./features.json').href), JSON.stringify(rest));
	const featuresWritePromise = fsa.write(fsa.toUrl('./features.json'), JSON.stringify(rest));

refactor: Specify common types TDE-1030 #849

refactor: Specify common types TDE-1030 #849

Conversation

l0b0 commented Feb 1, 2024 • edited Loading

Motivation

Checklist

blacha Feb 6, 2024

Choose a reason for hiding this comment

l0b0 Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

blacha Feb 6, 2024

Choose a reason for hiding this comment

l0b0 Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

blacha Feb 11, 2024

Choose a reason for hiding this comment

l0b0 Feb 12, 2024

Choose a reason for hiding this comment

blacha Feb 13, 2024

Choose a reason for hiding this comment

blacha left a comment • edited Loading

Choose a reason for hiding this comment

blacha Feb 18, 2024

Choose a reason for hiding this comment

l0b0 Feb 19, 2024

Choose a reason for hiding this comment

l0b0 commented Feb 19, 2024

l0b0 commented Feb 1, 2024 •

edited

Loading

l0b0 Feb 7, 2024 •

edited

Loading

l0b0 Feb 7, 2024 •

edited

Loading

blacha left a comment •

edited

Loading