feat: NPM support by bcheidemann · Pull Request #26 · mpalmer/action-validator

bcheidemann · 2023-02-04T08:24:15Z

Closes #25

Adds support for use as an NPM/Node module.

TODO:

Run standard test suite in "node mode"
Build/test in QA workflow
Release workflow to deploy NPM package (with semver)
Use latest Valico from official repo (currently it's not published to crates.io yet)
Expose ergonomic node API (i.e. remove logs in favour of returning JsValue)

bcheidemann · 2023-02-04T08:32:45Z

@mpalmer I've added a TODO list above based on your previous comment. It would be good to get an idea of what you think the priority order should be for this. Also which of these you want to check off before merging this PR, and which (if any) you think can be handled separately? If you have any guidance/pointers on that would also be appreciated (I'm relatively new to Rust so this is a bit of a learning experience for me 🙂).

bcheidemann · 2023-02-04T09:13:11Z

I've been working on improving the Node API with some success but encountered an unexpected behaviour of valicos serialize trait. I've raised an issue to determine if this is intended behaviour or a bug.

mpalmer · 2023-02-04T10:10:33Z

I think the critical threshold is to know if/when a future change breaks the node build, so having the test suite run successfully under node, and have workflow jobs that run that test suite, are the "baseline", I think. Having an automated release process is a prerequisite for making WASM/node a "first class citizen", but could be deferred to a separate PR -- although I'm not sure how much use a WASM-capable action-validator would be without released packages, so my hunch would be that would go into this PR (or one that would be expected to land more-or-less in parallel). Ergonomic improvements could be deferred to a later time, as long as node users are happy with API breakage, or the changes are backwards-compatible.

In summary, then, I'd say the order is:

Make it build/function (seems like it's pretty much already done)
Make sure I don't break anything when I'm making other changes (test suite, QA jobs)
Ergonomic improvements that will be non-backwards compatible
Automated releases (so it can be widely used)
Further improvements to docs, API, etc

1 & 2 are the minimum for this PR, IMO; other parts could be deferred to other PRs, but I'm cool with you making this a Mega PR o' Doom if you prefer.

bcheidemann · 2023-02-15T17:52:59Z

@mpalmer Apologies for leaving this PR dangling. I've been away for work and haven't had much time to work on this.

I agree with the priorities as you set them out above. I think a Mega PR 'o Doom is probably the best way since, as you pointed out, we basically need to get to 4. for it to be useful anyway!

Current status of the above is:

1. Make it build/function

Yes, this is more or less done.

The only exception here (afaik) is that the glob function is not WASM friendly.

I'm thinking we can probably use a wrapper function with different implementations depending on the build type. This can call into JS to resolve the glob.

2. Test suite, QA jobs

It shouldn't be an issue to adapt/implement a runner for the current test suite, similar to the test/run script but asserting against snapshots of the returned JS object instead of stdout and stderr.

3. Ergonomic improvements that will be non-backwards compatible

EDIT: I've gone ahead and done the implementation as described below - hopefully it's ok but any feedback is very welcome! :)

I could do with a second opinion on this. It seems we have 3 main errors:

Missing files / invalid globs
Missing "needed" jobs
Schema validation

For 3. I was initially thinking to return the Valico's ValidationState hence raising this issue and later this PR.

However, I see 2 main disadvantages here:

The return type of the validate function would be... for lack of a better word, a bit weird, since ValidationState serializes to Maps instead of objects which doesn't fit well into the JS ecosystem.
We would need to somehow return additional errors "in parallel" with the schema ValidationState since we don't rely on Valico for validating 1. and 2. For example, we might return { schemaValidationState: { /* ValidationState */ }, otherErrors: [ /* array of other errors not provided by Valico */ ] }.

I think my preference for addressing this would be to implement our own ValidationState and ValidationError types internal to this package. This way we can maintain control over serialization and the structure. I think this could be used by the underlying validation logic, keeping the data structures in use consistent across the CLI binary and the NPM library. Additionally, it would make distributing action-validator as a crate trivial.

What are your thoughts on this?

4. Automated releases

I haven't started on this but I have some experience with GitHub actions and publishing NPM packages so this should be fairly straightforward.

Would you want to continue publishing to @bcheidemann/action-validator or deprecate it and publish a new package?

5. Further improvements to docs, API, etc

I'll tackle this once the API is finalised :)

mpalmer · 2023-02-16T06:43:04Z

Don't worry about leaving things hanging; we've all got things to do.

The only exception here (afaik) is that the glob function is not WASM friendly.

Interestingly, there's recently been another bug report about how the current globbing support is out-of-step with GitHub's docs. I am in no way married to the current crate for globbing, so if you find one that's WASM-friendly and better than the current one (in terms of GitHub compatibility) I would be entirely open to a switch.

I think my preference for addressing this would be to implement our own ValidationState and ValidationError types

I'll level with you: the "failure reporting" in action-validator was originally "the simplest thing that could possibly work", and I've never been real happy with it. I'd be entirely open to your turning the errors that come out of Valico into something a little more... sensible, and returning those into the UI for more human-readable error reporting. The CLI error reporting is not, IMO, a backwards-incompatible interface (as in, if the CLI output changes, that's fine), so feel free to knock yourself out -- even if it's just a dump of the ValidationError type, rather than the error that Valico spits back, that's OK (if someone in the future wants a cleaner error report, "PRs welcome").

Would you want to continue publishing to @bcheidemann/action-validator or deprecate it and publish a new package?

I lean towards publishing it under a namespace that "the project" controls, so I'd probably go with @action-validator/action-validator, if that's the usual approach that NPM projects adopt. Does that seem reasonable and in line with NPM norms?

bcheidemann · 2023-02-16T13:11:08Z

nterestingly, there's recently been #27 about how the current globbing support is out-of-step with GitHub's docs. I am in no way married to the current crate for globbing, so if you find one that's WASM-friendly and better than the current one (in terms of GitHub compatibility) I would be entirely open to a switch.

Ah good to know! Maybe an opportunity to kill two birds with one stone :)

I'll level with you: the "failure reporting" in action-validator was originally "the simplest thing that could possibly work", and I've never been real happy with it. I'd be entirely open to your turning the errors that come out of Valico into something a little more... sensible, and returning those into the UI for more human-readable error reporting. The CLI error reporting is not, IMO, a backwards-incompatible interface (as in, if the CLI output changes, that's fine), so feel free to knock yourself out -- even if it's just a dump of the ValidationError type, rather than the error that Valico spits back, that's OK (if someone in the future wants a cleaner error report, "PRs welcome").

Thanks for confirming :) The simplicity is definetly appreciated as somone relatively new to Rust - it's very easy to understand what the code is doing!!

I lean towards publishing it under a namespace that "the project" controls, so I'd probably go with @action-validator/action-validator, if that's the usual approach that NPM projects adopt. Does that seem reasonable and in line with NPM norms?

My initial thought was to publish it under action-validator but a) that's already taken and b) I think it's good to have an NPM scope as you suggested so future packages can be published under the same scope. Maybe we could go with something like @action-validator/lib? Then we can also publish @action-validator/cli for a CLI version of this package distributed via NPM.

bcheidemann · 2023-02-16T00:08:57Z

src/validation_error.rs

+pub enum ValidationError {
+    // Schema Validation Errors
+    WrongTypeSchemaError {
+        meta: ValidationErrorMetadata,


I think it would be better to remove the meta field and elevate all the meta.* properties to the top level of the error. I've implemented like this for now but I'm intending to re-factor it.

Is this re-factor still planned, for this or a future PR?

I would like to. Tbh I was procrastinating because I wasn't sure what the best way would be, since I wanted a solution which would work well for serialising to JS but also makes it easy to work with in Rust in case this is distributed as a crate.

For me, the current front runner solution would be:

'''rs
enum ValidationError {
SomeError {
path: String,
...fields
}
}

impl ValidationError {
pub fn get_path(): String {
...
}
}
'''

Since this allows matching over error types but also just quickly grabbing the value of a given field depending on the use case.

Let me know if you think this is ok or if you have another suggestion for how we couple refactor this :)

bcheidemann · 2023-02-16T00:13:06Z

test/004_failing_globs/stderr

+Validation failed: ValidationState {
+    errors: [
+        NoFilesMatchingGlobError {
+            meta: ValidationErrorMetadata {
+                code: "glob_not_matched",
+                path: "/on/push/paths",
+                title: "Glob does not match any files",
+                detail: Some(
+                    "Glob \"004_bad_globs/*.txt\" in /on/push/paths does not match any files",
+                ),
+            },
+        },
+    ],
+}


Because all the errors are now handled internally using the same data-structures, I have removed any special cases for now and just logged the validation state. I think it would be nice to implement a pretty printer for errors - maybe something that could be controlled by a CLI flag e.g. --format=pretty, --format=json, --format=debug. What do you think?

I'll never say "no" to UI improvements, if you're willing to code them up, but in the interests of getting the WASM support landed, perhaps it'd be best to defer pretty-printing for a future PR. 😁

bcheidemann · 2023-02-16T20:23:47Z

CODE_OF_CONDUCT.md

+- The use of sexualized language or imagery
+- Personal attacks
+- Trolling or insulting/derogatory comments
+- Public or private harassment
+- Publishing other's private information, such as physical or electronic


There's some unrelated changes from running prettier. I can configure prettier to only run on JavaScript files and revert the changes if you prefer @mpalmer.

Yes, it would be best if you didn't modify the CoC file -- it's taken directly from upstream, and reformatting would make it harder to incorporate changes from upstream in the future.

Cool, I will add it to the .prettierignore file and revert the changes :) prettier also formats any other YAML, JSON and Markdown files by default. Let me know if you want me to disable it - I personally don't mind it but appreciate it could be annoying especially given it will fail the CI.

Changes reverted and CODE_OF_CONDUCT.md added to .prettierignore

mpalmer · 2023-02-16T22:19:14Z

Maybe we could go with something like @action-validator/lib? Then we can also publish @action-validator/cli for a CLI version of this package distributed via NPM.

If that's a common idiom in the NPM ecosystem, I'm fine with going with that.

bcheidemann · 2023-02-17T23:31:14Z

@mpalmer I think this is now ready for review :)

I haven't made any changes to glob yet since this PR is already quite big and I figured maybe we can defer the glob implementation to a separate PR. Would you be happy with this? I also didn't do any changes to the UI yet since I will defer this to another PR as you suggested 👍 I can't think of anything else that needs to be done but let me know if I've missed something!

Once this is merged, you will need to create the NPM organisation "action-validator" and add NPM_TOKEN as a secret for the repo. Then you can create a release and it will automatically publish both the core and cli packages to NPM with the release version.

Technically, since the two packages deploy in parallel and core has a peer dependency on cli, it could be that someone tries to install the latest version of cli but the latest version of core is not yet published, in which case things would break a bit... I am happy to fix this as part of this PR if you want or can do it in a follow-up.

mpalmer

This is a monster PR, and nice work getting through it. I've dropped a couple of questions, but I can't see anything that needs changing.

For the merge, I like to have a relatively "clean" commit history -- while multiple commits in a PR are cool, when they touch different things or are for different purposes, I prefer to git rebase -i and squash "fixup" / "tidyup" commits into the commits they're fixing, to keep the commit history more readable. To put it another way, I like PRs to represent the finished thing, rather than the "stream of consciousness" of changes as they happened.

Of course, since I do those cleanups to the commit history of my own PRs, I understand that it can be a PITA to do a git rebase -i+selective squash on a huge PR like this, so the other alternative is just to squash everything into one commit and land that. If you'd prefer to go that way, I can do that when I merge the PR. Otherwise, tidy up the commit history and force-push to the PR branch, and I can merge the branch clean once the open questions are answered.

src/validation_error.rs

mpalmer · 2023-02-23T23:53:12Z

src/validation_error.rs

+pub enum ValidationError {
+    // Schema Validation Errors
+    WrongTypeSchemaError {
+        meta: ValidationErrorMetadata,


Is this re-factor still planned, for this or a future PR?

bcheidemann · 2023-02-24T08:57:56Z

@mpalmer Thanks for reviewing this 🙂

Normally I am a proponent of squash on merge since I prefer to keep my PRs small and focussed, but in this case I think it makes sense to break it up a bit.

I have quite a busy schedule this weekend but I'll try and address the implementation of ValidationError and tidy up the commits if I can make time or if not then ASAP next week.

mpalmer · 2023-02-25T00:12:27Z

CONTRIBUTING.md

@@ -1,10 +1,10 @@
-* If you have found a discrepancy in documented and observed behaviour, that


Sorry to put another thing on you, but I've pondered these (presumably prettier-inspired) formatting changes a bit longer, and I've decided that I can't live with prettier trying to copy-edit my markdown. I'll be spending the rest of my life making "make prettier happy" commits, to comply with a markdown style that I find pretty unpleasant and unnatural. Please revert the formatting-only changes to markdown files, and teach prettier to leave them alone for the future. Thanks.

I'll get prettier to ignore everything that isn't a JavaScript file (since that's what I originally added it for) and revert all other changes

bcheidemann · 2023-02-25T22:46:50Z

@mpalmer I've made the requested changes and tidied up the commit history =)

mpalmer

I've got one question about a recent feature that I worry you might have nuked, but apart from that, your changes look reasonable, and ready to merge as soon as we've cleared up the merge issue.

mpalmer · 2023-02-27T05:11:46Z

src/main.rs

+    use clap::Parser;
+    use std::process;

-    // Run the validator on each file and collect any errors


I can't seem to find where this code has gotten to. I suspect your merge conflict resolution might have nuked it.

Yeah the merge conflicts were pretty bad - there was basically no overlap 😬 this code has moved to cli::run but looks a bit different now. Functionality is the same as far as I can tell from a few manual tests. I can refactor to make it more similar if that's preferable and/or add some automated tests for the new functionality?

I really should have put automated tests in when #31 was merged, shouldn't I... I'll add one to main now, for cleanliness, and then if your PR is still clean against that, I think we're ready for merge.

Exellent! Thanks for picking that up =) Let me know if there's anything I can do to help

mpalmer · 2023-03-03T02:59:58Z

WE ARE LANDED! I'll create an NPM account/key/etc, add it as a repo secret, and then make a release to see this puppy fly!

mpalmer · 2023-03-03T05:43:05Z

The release CI job seemed to run OK, so I presume that everything is hunky dory in NPM land. Winnah!

bcheidemann · 2023-03-03T23:55:44Z

The release CI job seemed to run OK, so I presume that everything is hunky dory in NPM land. Winnah!

I've tested it out and it seems to be working perfectly :) Thanks for your support and guidance on this @mpalmer!

There's a couple things which were left out of this PR that I'll probably look into soon if you're happy to accept PRs for them?

Multi-file support for @action-validator/cli
Glob validation support for @action-validator/* (and possibly addressing paths wildcard validation seems off #27)

mpalmer · 2023-03-05T23:28:18Z

More than happy to accept PRs for both of those. Also, what's the chances of getting the testsuite to use @action-validator/cli when run in "WASM mode", rather than reimplementing all the logic from the run shell script in run.mjs? I notice that the JS CI job is failing, because of 009_multi_file, and rather than adding multi-file support to run.mjs, it'd be "cleaner" to just use the CLI package to run equivalently to the native binary CLI, and avoid duplicating the test runner logic. WDYT?

bcheidemann · 2023-03-06T00:21:28Z

@mpalmer I completely agree. Initially I hadn't planned to add @action-validator/cli, hence the need for run.mjs. But now that it exists, it would be better not to have two test harasses doing essentially the same job.

For now, I have raised a PR to update the snapshot so the CI will pass (#34).

When I add multi-file support to @action-validator/cli then I will also remove run.mjs and get run to use @action-validator/cli when running in WASM mode as suggested.

bcheidemann commented Feb 16, 2023

View reviewed changes

mpalmer reviewed Feb 23, 2023

View reviewed changes

mpalmer reviewed Feb 25, 2023

View reviewed changes

bcheidemann added 5 commits February 25, 2023 22:42

feat: Setup @action-validator/core NPM package

f76417b

feat: Setup @action-validator/cli NPM package

c57bedb

test: Add tests for @action-validator/core NPM package

26b1fde

ci: Update QA and release workflows for NPM packages

a3db81b

docs: Update docs for NPM packages

5ec7714

bcheidemann force-pushed the feat/npm branch from af7dbc2 to 5ec7714 Compare February 25, 2023 22:45

bcheidemann requested a review from mpalmer February 25, 2023 22:45

mpalmer reviewed Feb 27, 2023

View reviewed changes

bcheidemann requested a review from mpalmer February 27, 2023 13:57

mpalmer merged commit 710bf60 into mpalmer:main Mar 3, 2023

		@@ -1,10 +1,10 @@
		* If you have found a discrepancy in documented and observed behaviour, that

Uh oh!

Conversation

bcheidemann commented Feb 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bcheidemann commented Feb 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bcheidemann commented Feb 4, 2023

Uh oh!

mpalmer commented Feb 4, 2023

Uh oh!

bcheidemann commented Feb 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. Make it build/function

2. Test suite, QA jobs

3. Ergonomic improvements that will be non-backwards compatible

4. Automated releases

5. Further improvements to docs, API, etc

Uh oh!

mpalmer commented Feb 16, 2023

Uh oh!

bcheidemann commented Feb 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpalmer commented Feb 16, 2023

Uh oh!

bcheidemann commented Feb 17, 2023

Uh oh!

mpalmer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bcheidemann commented Feb 24, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bcheidemann commented Feb 25, 2023

Uh oh!

mpalmer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpalmer commented Mar 3, 2023

Uh oh!

mpalmer commented Mar 3, 2023

Uh oh!

bcheidemann commented Mar 3, 2023

Uh oh!

mpalmer commented Mar 5, 2023

bcheidemann commented Feb 4, 2023 •

edited

Loading

bcheidemann commented Feb 4, 2023 •

edited

Loading

bcheidemann commented Feb 15, 2023 •

edited

Loading