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

feat(config-file): provide support for config file #62

Merged
merged 10 commits into from
Oct 4, 2022
Merged

Conversation

GervinFung
Copy link
Collaborator

This PR is to resolve the issues stated in #55

Copy link
Owner

@garronej garronej left a comment

Choose a reason for hiding this comment

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

Thank you very much for your hard work.
Here is my review.

src/lib/denoify.ts Outdated Show resolved Hide resolved
src/lib/denoify.ts Outdated Show resolved Hide resolved
src/lib/parseParams.ts Outdated Show resolved Hide resolved
src/lib/parseParams.ts Outdated Show resolved Hide resolved
src/lib/parseParams.ts Outdated Show resolved Hide resolved
src/lib/parseParams.ts Outdated Show resolved Hide resolved
src/lib/parseParams.ts Outdated Show resolved Hide resolved
src/lib/parseParams.ts Outdated Show resolved Hide resolved
src/test/index.ts Outdated Show resolved Hide resolved
tsconfig.build.json Outdated Show resolved Hide resolved
@garronej
Copy link
Owner

Also it would be realy nice to write some doc for the available config on https://docs.denoify.land like I have done with https://docs.keycloakify.dev/build-options.
I sent you an invite for the GitBook by email.

@GervinFung
Copy link
Collaborator Author

Also it would be realy nice to write some doc for the available config on https://docs.denoify.land like I have done with https://docs.keycloakify.dev/build-options. I sent you an invite for the GitBook by email.

Can you resend it to me? I dont think I have the invitation for the GitBook

@garronej
Copy link
Owner

@GervinFung LGTM! Thanks for applying my suggesting and refactorying!

@GervinFung
Copy link
Collaborator Author

@GervinFung LGTM! Thanks for applying my suggesting and refactorying!

Just to confirm that there is nothing left to resolve right? If that's the case, I will fix the merging issues and merge it if you are ok with it

@garronej
Copy link
Owner

Just to confirm that there is nothing left to resolve right? If that's the case, I will fix the merging issues and merge it if you are ok with it

Yes alright, let's release in beta first to make sure nothing've been broken and until there is some documentation for it.

So just fix the conflict and update the package.json version to 1.2.0-beta.0, it will release a beta on NPM and github release.

Comment on lines +1 to +35
import { ModuleAddress } from "../../../lib/types/ModuleAddress";
import { assert } from "tsafe";
import { getValidImportUrlFactory } from "../../../lib/resolveNodeModuleToDenoModule";

(async () => {
const userOrOrg = "GervinFung";
const moduleAddress: ModuleAddress.GitHubRepo = {
"type": "GITHUB REPO",
userOrOrg,
"repositoryName": "my_dummy_npm_and_deno_module",
"branch": "master"
};

{
const getValidImportUrlFactoryResult = await getValidImportUrlFactory({
moduleAddress,
"desc": "NOT LISTED AS A DEPENDENCY (PROBABLY NODE BUILTIN)"
});

assert(getValidImportUrlFactoryResult.couldConnect === true);

assert(getValidImportUrlFactoryResult.versionFallbackWarning === undefined);

assert(
(await getValidImportUrlFactoryResult.getValidImportUrl({ "target": "DEFAULT EXPORT" })) ===
`https://raw.githubusercontent.com/${userOrOrg}/my_dummy_npm_and_deno_module/master/deno_dist/mod.ts`
);
assert(
(await getValidImportUrlFactoryResult.getValidImportUrl({ "target": "SPECIFIC FILE", "specificImportPath": "dist/lib/Cat" })) ===
`https://raw.githubusercontent.com/${userOrOrg}/my_dummy_npm_and_deno_module/master/deno_dist/lib/Cat.ts`
);
}

console.log("PASS");
})();
Copy link
Owner

Choose a reason for hiding this comment

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

This test is a very good idea!
A couple of thing I would like to be addressed:

  • Could you create an other fork and an other test (13) where your config file would have an explicite a out parameter to make sure that your file is wel parsed.
  • Create a GitHub release for your forks, and use:
            "desc": "MATCH VERSION INSTALLED IN NODE_MODULES",
            "version": "v1.0.0"

instead of NOT LISTED AS A DEPENDENCY (PROBABLY NODE BUILTIN), it's a bit confusing, it make it hard to understand what we are testing.

Thank you very much!

@garronej garronej merged commit 9d05bd5 into main Oct 4, 2022
@garronej garronej deleted the config-file branch October 4, 2022 19:54
garronej added a commit that referenced this pull request Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants