fix: constructor validation on build #226
Conversation
|
Output from CI: |
|
@osalkanovic you can add tests that can expect something to fail or not to fail, check examples here: https://github.com/near/near-sdk-js/blob/develop/tests/__tests__/decorators/payable.ava.js The fact that the constructor exists does not mean that all the variables were initialized in it. So users can get errors anyway. |
…ear-sdk-js into fix/constructor-validation
…/constructor-validation
|
And ya, |
There was a problem hiding this comment.
@osalkanovic wow, it's a great approach! I think this solution is great. Let's address a few comments and merge it.
P.S. Just run yarn lint and yarn format before the commit. It will fix most of the linter errors.
| t.is(result.status, 2); | ||
| }); | ||
|
|
||
| test("should throw error, construcor isnt declared", async (t) => { |
There was a problem hiding this comment.
It would be great to compare error text here. Can we do that?
There was a problem hiding this comment.
I think result.stdout / result.stderr records the error text
There was a problem hiding this comment.
The validation error is overridden with another top-level error. Added const for now.
cli/contract_validation.js
Outdated
| if (hasBindgen) { | ||
| const constructors = contractClass.getConstructors(); | ||
| const hasConstructor = constructors.length > 0; | ||
| const propertiesToBeInited = properties.filter((p) => !p.initializer); |
There was a problem hiding this comment.
How does it work with JS contracts?
There was a problem hiding this comment.
Do not work for JS contracts due to the absence of parameter declaration.
cli/cli.js
Outdated
| @@ -86,6 +86,7 @@ async function checkTsBuildWithTsc(sourceFileWithPath) { | |||
|
|
|||
| // Common build function | |||
| async function createJsFileWithRullup(sourceFileWithPath, rollupTarget) { | |||
| await validateContract(sourceFileWithPath); | |||
There was a problem hiding this comment.
Nice step! All existing contracts can pass the validation
|
Hey @osalkanovic ! This PR is getting outdated, it will be hard to continue your work on it if we postpone it. Do you have the capacity to continue doing this? |
volovyks
force-pushed
the
fix/constructor-validation
branch
from
401453c to
7e0eb28
Compare
|
@volovyks Looks great 👍 The original functionality by @osalkanovic is good, and your refactors, rewrote tests and in TS makes the code quality better than I expected. Merge now |
|
@osalkanovic thanks for the help! |
#224
I have commented unordered map constructor to simulate issue and I expect to CI fail.
When someone review PR I will return previous state of unordered map :)