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

Implement module packing using ncc #18

Merged
merged 4 commits into from
Aug 6, 2024

Conversation

Yakov5776
Copy link

Reason has been brought up in #13 (comment)

This has to be removed since `ncc` produces different/randomized constant output each time and will be different from upstream.
@Yakov5776
Copy link
Author

I know it seems like a lot of changes, this is basically what I did:

  • added ncc package to the testing workflow
  • added ncc to the build process in packages.json (replacing tsc)
  • since ncc uses /lib/index.js i renamed /lib/main.js to that in action.yml
  • deleted all files in /lib other than the single index.js file which gets generated
  • removed the check in the testing workflow which checks if there's any lingering commits not comitted because ncc output changes due to randomization and that causes false positive of missed-build ec4e342

things to mention:

  • i don't really know what jest is or if it's still needed, it seems to be 5yr old testing remnants but it seems the workflow does the job instead (?)
  • i don't know if eslint is even needed since ncc does it's own thing with source formatting as it's job is to pack all modules into a single file so i don't know if we really need it
  • what's .husky/pre-commit?

in general i do think we could debloat this repository a lot from all the mostly useless quirks / packages, let me know what your thoughts.

also if it wasn't clear from my last comment, right now this repository upstream is non-functional, meaning any workflow targetting this action from main or 2.0.0 won't work which is why this merge is kind of urgent.

and last but not least, let's be consistent on our versioning scheme, is it v2.0.0 or v2? latest tag and readme aren't the same.

@Yakov5776
Copy link
Author

side question - could i become a maintainer?

Copy link
Owner

@kevin-david kevin-david left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few comments. I unpublished v2.0.0 in the meantime. Overall this seems like a good change though!

.github/workflows/release.yml Outdated Show resolved Hide resolved
lib/tests/io-utils.test.js Show resolved Hide resolved
@kevin-david kevin-david merged commit b23618d into kevin-david:main Aug 6, 2024
3 checks passed
@Yakov5776
Copy link
Author

Are we doing a new release candidate? @kevin-david

@kevin-david
Copy link
Owner

@Yakov5776 just pushed it and created two tags, v2.0.1 and v2

@Yakov5776 Yakov5776 deleted the ncc-migration branch August 6, 2024 02:34
@kevin-david
Copy link
Owner

removed the check in the testing workflow which checks if there's any lingering commits not comitted because ncc output changes due to randomization and that causes false positive of missed-build ec4e342

I would like to figure this out somehow. I don't love the idea of having to review generated javascript (error prone / supply chain attacks). Maybe we just leave /lib out except in release branches/commits? Not sure, still thinking about it.

i don't know if eslint is even needed since ncc does it's own thing with source formatting as it's job is to pack all modules into a single file so i don't know if we really need it

eslint was set up to establish consistent/automatic following conventions in the .ts files

what's .husky/pre-commit?

this is what's called a pre commit hook (along with some tooling) so all commits follow the same style - just makes for easier reviews & less arguments about style 😄

@Yakov5776
Copy link
Author

Yakov5776 commented Aug 6, 2024

I would like to figure this out somehow. I don't love the idea of having to review generated javascript (error prone / supply chain attacks). Maybe we just leave /lib out except in release branches/commits? Not sure, still thinking about it.

I don't like that idea either, but in terms of the extent of ncc customization, i don't see a way in making it produce consistent results across the board. Also, is there a reason the issues tab is closed?

@Yakov5776
Copy link
Author

Yakov5776 commented Aug 6, 2024

I'm just brainstorming. Maybe potentially have a different branch for distribution/deployments which automatically merges any changes from development/main branch powered by a workflow and does the building by itself and only that branch will even have the lib folder, and the main branch will have it git ignored. @kevin-david what do you think? After some googling, I found out it is possible to create releases/tags from a non-default branch.

@kevin-david
Copy link
Owner

Turns out other people have run into this problem: vercel/ncc#831

#19 fixes it, between my machine and Actions at least. If you run npm run build locally do you have any pending changes now?

@Yakov5776
Copy link
Author

oh sick, let me try

@Yakov5776
Copy link
Author

Yakov5776 commented Aug 6, 2024

#19 fixes it, between my machine and Actions at least. If you run npm run build locally do you have any pending changes now?

Yup it works, you might want to add a .gitattributes to the repository like * text=auto eol=lf because my Windows machine wants to re-encode it to CRLF.

@kevin-david
Copy link
Owner

#19 fixes it, between my machine and Actions at least. If you run npm run build locally do you have any pending changes now?

Yup it works, you might want to add a .gitattributes to the repository like * text=auto eol=lf because my Windows machine wants to re-encode it to CRLF.

Good idea - #21

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