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

Sm/strict-mode #475

Merged
merged 8 commits into from
May 16, 2023
Merged

Sm/strict-mode #475

merged 8 commits into from
May 16, 2023

Conversation

mshanemc
Copy link
Contributor

@mshanemc mshanemc commented May 9, 2023

What does this PR do?

strict mode
a little less classy
fewer ts assertions
sketchy injectable fsImplt => sinon
reduce eslint comments

What issues does this PR fix or reference?

@W-12220467@
@W-13183108@

@mshanemc mshanemc requested a review from WillieRuemmele May 9, 2023 22:42
@@ -284,24 +267,36 @@ export class InstallationVerification implements Verifier {
public async streamTagGz(): Promise<NpmMeta> {
const logger = await this.getLogger();
const npmMeta = await this.retrieveNpmMeta();
if (!npmMeta.tarballUrl) {
Copy link
Member

Choose a reason for hiding this comment

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

seems safe to update the npm meta type to account for tarball to always be present in the response.
https://github.com/verdaccio/verdaccio/blob/d52dbadae84edb1c34905bf09c3e8f8bf708eed0/packages/core/types/src/manifest.ts#L74

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right, but 😢 we've got a public constructor on an exported class, which would become invalid if the tarballUrl is required.

public constructor(private module: string, private version: string = 'latest', private cliRoot?: string) {
    this.npmMeta = {
      moduleName: module,
    };
  }

This is kinda like what I showed in demos with classes--all the props are optional and the consumers has to know which methods need be called before which props/methods become valid.

Copy link
Member

@cristiand391 cristiand391 left a comment

Choose a reason for hiding this comment

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

Looks good, left 2 comments but just 1 request change for the unhandled error.

@cristiand391
Copy link
Member

QA notes:

🟢 successfully verify signature, defaults to npm registry if -r/--registry isn't specified.
DEBUG=*InstallationVerification* sf plugins trust verify --npm @salesforce/plugin-user

🟢 JSON output matches current sf.

🟢 allowlist file is respected by signature verification hook.

@cristiand391 cristiand391 merged commit 7142497 into main May 16, 2023
@cristiand391 cristiand391 deleted the sm/strict-mode branch May 16, 2023 15:24
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.

2 participants