-
-
Notifications
You must be signed in to change notification settings - Fork 3
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
Typescript Compliance #55
Conversation
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
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.
I forgot how many bullet points were in this ticket 😅 Good work on refactoring some of these rules. I left some suggestions on how we can simplify that code even further potentially. I need to still go through the test files and check them but this should be enough for now I imagine!
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.
Getting close! All of the logic looks correct to me, but I do want to make sure that the deep containment logic is being tested appropriately.
buildPackageManifestMock(), | ||
); | ||
const project = buildMetaMaskRepository({ | ||
shortname: 'project', | ||
directoryPath: path.join(sandbox.directoryPath, 'project'), | ||
}); | ||
await writeFile( | ||
path.join(project.directoryPath, 'package.json'), | ||
buildPackageManifestMock(), |
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.
What do you think about adding main
to the set of properties we're passing here? Same for below:
buildPackageManifestMock(), | |
); | |
const project = buildMetaMaskRepository({ | |
shortname: 'project', | |
directoryPath: path.join(sandbox.directoryPath, 'project'), | |
}); | |
await writeFile( | |
path.join(project.directoryPath, 'package.json'), | |
buildPackageManifestMock(), | |
buildPackageManifestMock({ | |
main: 'main.js', | |
}), | |
); | |
const project = buildMetaMaskRepository({ | |
shortname: 'project', | |
directoryPath: path.join(sandbox.directoryPath, 'project'), | |
}); | |
await writeFile( | |
path.join(project.directoryPath, 'package.json'), | |
buildPackageManifestMock({ | |
main: 'main.js', | |
}), |
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.
It is already there as a default set of properties. The suggested change may make it easy for the reader to understand what we are exactly passing as a main property! But at the same time, for some, it may also confuse, when it is already there, why this test is overriding with something else, when there's no need.
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.
I suppose it depends on how familiar the reader is with these tests. Someone who is familiar may not feel the need, if they already know that buildPackageManifestMock
includes main.js
in its list of defaults. I think it will be more common that these tests will appear fresh to even us, so the easier we can make it for ourselves to understand the test, the better.
If you think it would be confusing, though, then would it work to choose a different value in this case? Something like index.js
instead of main.js
, perhaps.
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.
A few more minor things. I did test this out though and everything seems to work!
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
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.
Looks good!
We want to make sure that for a given project:
If project's package.json contains a lavamoat field, and that contains allowScripts, then a tsup>esbuild property should be present and should match the same one in the module template.
Fixes #8