-
Notifications
You must be signed in to change notification settings - Fork 67
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
Dependency fix on typescript CLI #1180
Dependency fix on typescript CLI #1180
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unit test for the dependencies file
@@ -9,7 +11,7 @@ import { ResultAsync } from "neverthrow"; | |||
const parseDataUrl = require("data-urls"); | |||
const fsReadFile = util.promisify(fs.readFile); | |||
|
|||
const DataUrl = z.string().trim().transform((val, ctx) => { | |||
export const DataUrl = z.string().trim().transform((val, ctx) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exposing this objects so they can be unit tested
@@ -21,7 +23,7 @@ const DataUrl = z.string().trim().transform((val, ctx) => { | |||
return parsed; | |||
}); | |||
|
|||
const FileUrl = z.string().trim().url().transform((val, ctx) => { | |||
export const FileUrl = z.string().trim().url().transform((val, ctx) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exposing this objects so they can be unit tested
owner: z.string(), | ||
repo: z.string(), | ||
baseUrl: z.string().optional() | ||
}); | ||
|
||
const GithubConfig = z.union([GithubData, z.string()]); | ||
|
||
const DependencySettings = z.union([DataUrl, FileUrl, z.string().trim()]) | ||
// const DependencySettings = z.union([DataUrl, FileUrl, z.string().trim()]) | ||
const DependencySettings = z.string().trim(); | ||
const Dependencies = z.array(DependencySettings).default([]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the fix for the Dependencies
@@ -112,7 +129,7 @@ export async function loadAllDependencies(config: DependencyConfig) { | |||
const finalResults = await Promise.all(results); | |||
return finalResults.flatMap((result) => { | |||
if (result.isOk()) { | |||
console.error("Successfully loaded dependency", result.value.dependency) | |||
console.info("Successfully loaded dependency", result.value.dependency) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removing error messages for debugging purposes
@@ -46,7 +46,8 @@ async function make( | |||
const dependencyConfig = DependencyConfig.parse({ | |||
dependencies: morphirJson.dependencies, | |||
localDependencies: morphirJson.localDependencies, | |||
includes: includes | |||
includes: includes, | |||
projectDir: projectDir | |||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ProjectDir preserves the project directory option passed through the command line
expect(fileData).toBeUndefined | ||
}); | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dependencies test helps enforce the business logic of those types, ie the Url type does not support anything but ftp and http(s)
import { Readable } from "stream"; | ||
import { ResultAsync } from "neverthrow"; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Organizing imports
url?: URL, | ||
|
||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used by LocalFile to keep track of the different forms of a path
import { decode, labelToName } from "whatwg-encoding"; | ||
import { z } from "zod"; | ||
import { fetchUriToJson } from "./get-uri-wrapper"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get-uri-wrapper works a facade so HTTP types can be mocked.
@@ -214,7 +214,7 @@ async function testUnit(cb) { | |||
} | |||
|
|||
async function compileCli2Ts() { | |||
src('./cli2/*.ts').pipe(cliTsProject()).pipe(dest('./cli2/lib/')) | |||
src(['./cli2/*.ts', '!./cli2/*.test.ts' ]).pipe(cliTsProject()).pipe(dest('./cli2/lib/')) | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excluding unit tests from being copied to lib folder, otherwise the tes.js files will run , and fail, during integration tests
@@ -18,6 +18,7 @@ | |||
"ncc-morphir": "ncc build cli/morphir.js -o dist/morphir", | |||
"ncc-morphir-server": "ncc build cli/morphir-elm-develop.js -o dist/morphir-server", | |||
"build": "gulp && npm run ncc-morphir && npm run ncc-morphir-server && npx jest && gulp test", | |||
"build-cli2": "gulp buildCLI2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To speed up process of running integration tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR contains the following:
dependencies
property of morphir.json filelocalDependencies
compatibilityTerms