feat(config-cli): New config-cli package includes dump basemaps screenshots command line tool #2231
Conversation
| import 'source-map-support/register.js'; | ||
| import { CommandScreenShot } from './screenshot.js'; | ||
|
|
||
| export class ScreenshotCommandLine extends BaseCommandLine { |
There was a problem hiding this comment.
lets call this BasemapsConfig as we will add more sub tools to it like the config import
bmc as the script name? or basemaps-config ?
| import { Browser, chromium } from 'playwright'; | ||
| import { CommandLineAction, CommandLineFlagParameter, CommandLineStringParameter } from '@rushstack/ts-command-line'; | ||
|
|
||
| const TileTest = [ |
There was a problem hiding this comment.
after this pull request it would be great to store these as a JSON file.
| await page.waitForTimeout(1000); | ||
| await page.waitForLoadState('networkidle'); | ||
| } else { | ||
| throw new Error('Not supported on production yet'); |
There was a problem hiding this comment.
this is now in production we can take this logic out.
packages/config-cli/bmc.js
Outdated
| @@ -0,0 +1,3 @@ | |||
| #!/usr/bin/env node | |||
There was a problem hiding this comment.
this file should be referenced by the bin field in the package.json, it also wont be published.
Its best to put this into ./bin/bmc.mjs (being specific is super helpful for these)
then add bin/ to the files exported and add a bin section to map it to basemaps-screenshot ?
| export class BasemapsConfig extends BaseCommandLine { | ||
| constructor() { | ||
| super({ | ||
| toolFilename: 'screenshot', |
There was a problem hiding this comment.
this should the same name as the bin file
| lng: number; | ||
| z: number; | ||
| } | ||
| interface TileTest { |
| defaultValue: './test-tiles/default.test.tiles.json', | ||
| }); | ||
|
|
||
| this.verbose = this.defineFlagParameter({ |
There was a problem hiding this comment.
verbose is handled by being a part of BaseCommandLine
./bmc --verbose screenshot
There was a problem hiding this comment.
All fixed. Can you give another look?
| const host = this.host.value ?? this.host.defaultValue; | ||
| const tag = this.tag.value ?? this.tag.defaultValue; | ||
| const tiles = this.tiles.value ?? this.tiles.defaultValue; | ||
| if (host == null || tag == null || tiles == null) |
There was a problem hiding this comment.
I don't think so. But we need a checking in typescript to parse them as string instead of string | undefiened
|
This pull request introduces 1 alert when merging a6e3544 into e7b6a9e - view on LGTM.com new alerts:
|
| Dump the screenshots from basemaps production | ||
|
|
||
| ```bash | ||
| ./bin/bmc.js screenshot |
There was a problem hiding this comment.
NIT: if its described as bin : { "bmc": ... } the actual command will be bmc once you npm install it
packages/config-cli/README.md
Outdated
| Dump the screenshots from different host and tag | ||
|
|
||
| ```bash | ||
| .bin//bmc.js screenshot --host HOST --tag PR-TAG |
packages/config-cli/package.json
Outdated
| "access": "public" | ||
| }, | ||
| "files": [ | ||
| "build/" |
| export class BasemapsConfig extends BaseCommandLine { | ||
| constructor() { | ||
| super({ | ||
| toolFilename: 'bin', |
| @@ -0,0 +1,16 @@ | |||
| #!/usr/bin/env node | |||
There was a problem hiding this comment.
this file should never be executed directly.
| } | ||
| } | ||
|
|
||
| new BasemapsConfig().run(); |
There was a problem hiding this comment.
I would move this .run() part into the bin file. that way this file could in theory be imported
| import { z } from 'zod'; | ||
|
|
||
| enum TileMatrixIdentifier { | ||
| NZTM = 'NZTM2000Quad', |
There was a problem hiding this comment.
Do we not have this as a enum somewhere else?
We should be specific here Nztm2000Quad so we know we are talking about nztm2000quad tms and not the other one.
No description provided.