feat: rewrite to pure js

[bxb100]

as mentioned in #80, this is a PR for rewriting to js

PS. I can't pass the default lint config, so I choose to use the GitHub action template eslint.yml

I tested Service Accounts and 1Password Connect locally using ⁠npm run local-action. If any reviewer is interested, you can test it locally by copying ⁠.env.example to ⁠.env.

[Xuanwo]

NOTE to other reviewers: This PR is large due to the generated dist files; however, the significant changes are quite minimal.

[bxb100]

@Xuanwo Thank you for the review. I have learned a lot from your comments about the CI check. I have added a new workflow, ⁠check-dist.yml, to ensure the generated dist is reliable.

[Xuanwo]

@Xuanwo Thank you for the review. I have learned a lot from your comments about the CI check. I have added a new workflow, ⁠check-dist.yml, to ensure the generated dist is reliable.

Thank you, mostly LGTM now!

[Xuanwo]

Inviting @edif2008 and @SimonBarendse for a review.

[SimonBarendse]

Hi folks, really appreciate your efforts on this! ❤️ 🙌 Definitely aligned with the direction we'd like to move in for this and other integrations. We've build SDKs exactly because we saw such large desire from all of you to build integrations, and friction from doing so with CLI (e.g. performance, support for self-hosted runners, Windows support and it removes complexity from the implementation, making it easier to maintain and less error-prone). Excited to see you already help us move in that direction! 🚀

A couple things we'll want to look at to get this merged and deployed:

• Backwards compatibility: Continuity is quite important to us. We realize many depend on this and other integrations in critical workflows, and want to avoid disruptions as much as possible. With that in mind, we'll want to have a look at whether we can find a path to keep this action backwards compatible and at v2. So far, desktop app integration is the only missing feature in SDK we've identified to make a fully backwards compatible migration possible. While we work on closing that gap in SDKs, to not block this work from moving forward and make those aforementioned improvements available when not running locally (probably most uses), shall we support both side-by-side for now? We can check for Service Account token and/or Connect token envvars to use the new SDKs, and fall back to requiring CLI when those are not present (and thus probably running local). Once SDKs support Auth Prompts (aka desktop app integration) we can fully move over to SDK-only solution. That'd be a fully backwards compatible migration and we could stick to v2 throughout.
• Dist: Not sure why this action is using dist. @edif2008 is currently out, but when he returns next week, would love to learn from him what our reasoning was for introducing and if we could move away from that now. This could remove the complexity of required checks and prevent this repo from bloating by checking in a new binary for the WASM build on every new SDK release. Binary is already 5MB right now, and will likely grow further when expanding SDKs with additional functionality. With frequent SDK releases can add up quite quickly.
• Atomic Commits / PRs: To make the changes easier to review and move through faster, could you separate out unrelated changes in separate commits and PRs? For example I believe the changes to CONTRIBUTING.md, integration rename, linting changes, .gitignore additions, TS Config changes and .env.example introduction could each go in separate PRs, unless there's a relation that I'm not understanding yet. In that case, could you explain these changes and how they relate to this rewrite to help us understand and review? Shout-out to @AleksandrHovhannisyan for writing a great blog on this topic: https://www.aleksandrhovhannisyan.com/blog/atomic-git-commits/ 🙌

[Xuanwo]

Thank you @SimonBarendse for the bravo comments!

@bxb100 led the entire effort, so I will leave the final decision to him on how to implement changes. I'm here to explain why we need dist/.

For every uses: actions/checkout@v4, the GitHub Actions runner will clone the specified action actions/checkout and directly execute its entrypoint. There is no build or compile setup involved, so each action must include a bundled index.js file for the action to function properly.

The entry point is defined here: https://github.com/1Password/load-secrets-action/blob/0a309926fa2e1d3844db604b984ea6214baa028b/action.yml#L14-L16

Additional documentation can be found here: https://docs.github.com/en/actions/sharing-automations/creating-actions/creating-a-javascript-action

[yafanasiev]

I am not sure if there are some dependencies in 1Password SDK that require ncc and wasm specifically, but I've had a great experience bundling JS GitHub Actions with esbuild. It's fast, supports tree shaking and the final bundle is smaller than NCC one.

[Xuanwo]

I am not sure if there are some dependencies in 1Password SDK that require ncc and wasm specifically

I'm guessing 1Password has written their logic in Rust, with the JS SDK being just a wrapper.

[SimonBarendse]

I'm guessing 1Password has written their logic in Rust, with the JS SDK being just a wrapper.

Yes, this is exactly it. We're using WASM to share a single implementation (in Rust) between all languages we support SDKs in. Right now that's Go, Python and JS, and we've built it with the intention to grow to more languages (in a scalable & sustainable way).

[SimonBarendse]

Thanks for the explanation on why dist is required @Xuanwo ! ❤️ That makes sense.

[bxb100]

update 11/23: force update with minimal changes. the original version lives in branch #82-bak

---

Thank you @Xuanwo, @SimonBarendse, and @yafanasiev for your reviews and discussions. I would like to provide some explanations and background information regarding the submitted content in this PR:

• .env.example is an example config used for local-action, which facilitates local testing directly.
• .eslintignore, .github/linters/.eslintrc.yml, package.json (deleting eslintConfig) are changes made because I couldn't use the original @1password/front-end-style/eslintrc.yml when submitting the code. Therefore, I switched to the configuration recommended by https://github.com/actions/typescript-action.
• __tests__, config/jest.config.js are to expand the original test set.
• .github/workflows/check-dist.yml, .node-version are used to check the consistency of the generated dist for every PR and PUSH as mentioned before. It requires executing npm run bundle, which includes format:write.
• CONTRIBUTING.md results from the pre-push hook from husky executing npm run validate(contain format). I apologize for its appearance in this submission.
• The changes to .github/linters/tsconfig.json and tsconfig.json will be reverted.
• Regarding the fallback to CLI when there are no environment variables, perhaps we can follow @Xuanwo 's suggestion and bump into v3, then iterate gradually. I understand this occurs mainly during local testing, but it can also be bypassed by setting the environment variables.
• About compatibility, the inputs in action.yml remain consistent with version v2. The environment variables OP_SERVICE_ACCOUNT_TOKEN, OP_CONNECT_HOST, and OP_CONNECT_TOKEN also remain unchanged.
• Based on my understanding of action uses ^1, I believe frequently changing the dist output will only affect users who do not use the major version and need to update manually (although, to be honest, I haven't seen this usage scenario).
• For the bundled submission of this PR, since it was converted from one of my projects ^2, I did not consider atomic submissions. I sincerely apologize for any inconvenience this may have caused.

[Xuanwo]

Backwards compatibility

Hi @SimonBarendse, I suggest we avoid taking this direction since switching from CLI-based to JS-based does introduce a breaking change. I can foresee multiple ways this action could fail. It's common and expected for GitHub Actions users to upgrade actions between major versions (every Node version bump will trigger one).

Adopting load-secrets-action@v3 would provide a much smoother transition for the community. Releasing v3 doesn't mean users will adopt it by default; they will continue using v2 unless they manually update their workflows.

I am willing to create a migration guide for users to follow if there are any break happened.

• For the bundled submission of this PR, since it was converted from one of my projects 2, I did not consider atomic submissions. I sincerely apologize for any inconvenience this may have caused.

Hi @bxb100, I understand the concern from @SimonBarendse's side. To make the PR review smoother, do you think it would be a good idea to simply remove all files except for package.json, src/, and dist/? We can gradually add other staffs back later (which makes it much easier to review without involving real logic).

[SimonBarendse]

I would like to provide some explanations and background information regarding the submitted content in this PR

Thank you @bxb100! 🙌 This is very helpful.

Followed up in separate issues

• ES Lint Config: I've opened https://github.com/1Password/load-secrets-action/issues/83 for us to follow up on this.

• Linting:

  CONTRIBUTING.md results from the pre-push hook from husky executing npm run validate(contain format). I apologize for its appearance in this submission.

  Thank you! This uncovered a gap in our linting in CI. We're following up in https://github.com/1Password/load-secrets-action/issues/84

Rewrite specific changes - Discussing in this PR

Backwards Compatibility

I suggest we avoid taking this direction since switching from CLI-based to JS-based does introduce a breaking change. I can foresee multiple ways this action could fail.

@Xuanwo could you help me see these too? I'm currently under the impression that the new version of this action built on SDK will be fully compatible with the current version built on CLI, after addition of Auth Prompts. As @bxb100 mentioned as well:

the inputs in action.yml remain consistent with version v2. The environment variables OP_SERVICE_ACCOUNT_TOKEN, OP_CONNECT_HOST, and OP_CONNECT_TOKEN also remain unchanged.

Could you please help me see and understand what I'm missing? What are the failures you foresee?

Dist

Yes, I'm less concerned about this now and okay with proceeding with checking in the WASM binary into the code here after @Xuanwo's explanation why this is necessary. Apologies that I not explicitly mentioned that before.

I assume and hope when using the action that GitHub only fetches the referenced commit and not the full Git history, similar to the checkout action, by using --depth=1. That prevents historic binaries slowing down the action and increasing required bandwidth.

So the larger repo size from historic binaries checked in would only affect new contributors checking out the full repo, including all historic WASM binaries. But it won't affect using the action.

With that in mind, I believe we can pull back in the changes to dist and your introduction of .github/workflows/check-dist.yml.

Code Review

I've requested a review from @Marton6 and @edif2008 for the specific code changes made in this PR.

[bxb100]

Thank you, @SimonBarendse, for the detailed explanation.

With that in mind, I believe we can pull back in the changes to dist and your introduction of .github/workflows/check-dist.yml.

I believe we can proceed after #83 #84 .

I assume and hope when using the action that GitHub only fetches the referenced commit and not the full Git history, similar to the checkout action, by using --depth=1. That prevents historic binaries slowing down the action and increasing required bandwidth.

base on https://github.com/actions/runner/blob/078eb3b381939ee6665f545234e1dca5ed07da84/src/Runner.Worker/ActionManager.cs#L784C1-L792C1 I think it direct download like https://api.github.com/repos/{action.NameWithOwner}/tarball/{action.Ref}