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

ci: add main workflow #60

Merged
merged 2 commits into from
Oct 26, 2024
Merged

ci: add main workflow #60

merged 2 commits into from
Oct 26, 2024

Conversation

abn
Copy link
Contributor

@abn abn commented Oct 25, 2024

Adding a simple workflow to perform lint checks, build checks and ensure package can publish. Rolled everything into one as separating them seemed overkill for the project for now.

@abn
Copy link
Contributor Author

abn commented Oct 25, 2024

@mattdavis90 is it intentional that you are storing build output in the repository? The publishing (dry-run) step is failing because pnpm build generates a build output that differs from what is in the repository.

@abn
Copy link
Contributor Author

abn commented Oct 25, 2024

I have created #61 making the assumption that build is not desired in the source tree. If that is merged this can simply be rebased to trigger the runs.

@mattdavis90
Copy link
Owner

The output is deliberate maintained to keep compatibility with JavaScript only users. The library was originally JS only and the primary user which is my NodeRed integration is also JS only so those files will need to remain. Thanks

@abn
Copy link
Contributor Author

abn commented Oct 25, 2024

@mattdavis90 ack, i will add a check to ensure they are regenerated.

Out of interest (since I am not a JS/TS dev), wouldn't these be better served perhaps as a release artifact as part of the GH release?

@mattdavis90
Copy link
Owner

Possibly. They need to make their way into the NPM bundle. I guess they probably don't need to be in the repo but they previously were and I wanted to maintain backwards compatibility

@abn
Copy link
Contributor Author

abn commented Oct 25, 2024

Updated with the following:

  1. Verify that files do not change after a build.
  2. Ensure build script includes a format.

@mattdavis90
Copy link
Owner

mattdavis90 commented Oct 26, 2024

Hi, I've merged the two branches that added/updated features. They now conflict with this branch. I'm thinking more about the build artefacts in the repo and think maybe you're right and we remove them. Happy to do that in this PR, without opening another.

@abn
Copy link
Contributor Author

abn commented Oct 26, 2024

Yeah the merge conflicts are not fun :).

Do you prefer to remove it here and then attach it to release?

In order to not break any existing users, how are these referred to presently? Raw url?

@mattdavis90
Copy link
Owner

It was the merge conflict last night that made me thing again about having the build results in the source tree.

I think the chances of anyone using a direct Git URL in their package.json should be pretty small and if they are then hopefully they'll understand why it's been removed from the source tree.

I think we removed the build directory as per #61 and then on release build bundle up the artefacts into the NPM package like before. pnpm release should handle that as long as the artefacts are present.

@abn
Copy link
Contributor Author

abn commented Oct 26, 2024

Done. Removed the build directory. We can modify the release artifacts as we need in #62.

@mattdavis90
Copy link
Owner

Looks great. Thank you so much!!

I'll merge this now, then prep for a release under #62

@mattdavis90 mattdavis90 merged commit ab73a1d into mattdavis90:master Oct 26, 2024
6 checks passed
@abn abn deleted the ci/main branch October 26, 2024 09:26
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