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

Contributing to the library while using a different version of Node.js or NPM could make unexpected changes to the published packages #11

Open
gridsystem opened this issue Sep 21, 2024 · 3 comments · Fixed by #13
Assignees
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers

Comments

@gridsystem
Copy link

Expected Behavior

If another contributor makes changes to the repository, the files generated by Rollup for distribution should reflect only those changes, and nothing to do with the contributor's environment.

Current Behavior

If I am using a different version of Node.js or NPM and make a small change to any of the files in the src directory, and run npm dev, the generated files could differ beyond the change I've made.

Possible Solution

  • Include brief contributing instructions
  • Instruct contributors to use Node Version Manager and to use a specified version of Node.JS (Ideally the latest LTS version for longevity)
  • Include the specified version of Node.JS in .nvmrc so that contributors can run nvm use in the project directory to automatically install and select that version of Node.JS
  • Include the specified version of Node.JS in the engines property of package.json
  • Instruct contributors to install the specified version of NPM e.g. npm install [email protected] -g
  • Include the specified version of npm in the engines property of package.json

Notes

  • This could make other contributors more confident that they won't destroy your library ;)
  • This would open the door to configuring a GitHub Action that automatically tags, releases and publishes new versions of the library on NPM after PRs are merged e.g. via Release Please
  • This could also open the door to automated tests

If you're keen, let me know and I can open a PR.

@humanbydefinition
Copy link
Owner

humanbydefinition commented Nov 12, 2024

Hey once more @gridsystem, I'd like to apologize once more for keeping you waiting for so long. I really appreciate your determination to help and I'm more invested in bringing this library forward a bit more again, especially the npm package part.

The Contributing part of this repository isn't fleshed out yet, but I'm happy and thankful for any contributions and pull requests. I'm also more than happy to honor contributors in some way by listing them somewhere on the repository.

When I'm done with my testing in regards to the newly bundled library, which should now also work when imported via npm, I'm happy to invest time into this issue here as well. But I'm also open to accept pull requests that solve this issue and provide some instructions for potential contributors.

I'm currently on Node.js version v20.11.1 and npm version 8.19.2, but since those do not seem to be LTS versions, I'm happy to make the switch to Node.js v22.11.0 (LTS) and whatever npm version is associated with it. :)

While I wasn't working on this library directly, I implemented a new ascii shader which might qualify as a new rendering mode apart from the existing brightness mode in the future. Essentially, it's a mode that attempts to find the most fitting ascii character from the given character set to produce the best possible ascii representation of an input image. In the video below is a video of me sucking at Rocket League a few years ago, which is accurately asciified in real-time at 60 frames per second. I really like the results, could also see this working as a overlay for video games in general. 😄

output_rltest.mp4

Also, this library currently is dependent on the viewers machine being compatible with WebGL2. In other ascii-related projects I did recently, I was able to get everything working in WebGL1, which also means support for older and more obscure devices. :) I might also add this to this library soon as well, to improve compatibility even further.

Cheers!

@humanbydefinition humanbydefinition added documentation Improvements or additions to documentation good first issue Good for newcomers labels Nov 13, 2024
@humanbydefinition humanbydefinition self-assigned this Nov 13, 2024
@humanbydefinition
Copy link
Owner

Hey again @gridsystem! I have started working on this in a separate branch, which is also linked in this issue: https://github.com/humanbydefinition/p5.asciify/tree/11-contributing-to-the-library-while-using-a-different-version-of-nodejs-or-npm-could-make-unexpected-changes-to-the-published-packages

I have been testing around a bit and really like the functionalities that come with the suggestions you provided, thanks a bunch!

Except updating the README and the Wiki potentially, I have already pushed solutions for all the other suggestions you have made. The workflow implemented is currently bare-bones and does some redundant checks I think, but overall it should be able to check whether the source is bundled successfully and is not empty.

I'd love to have your feedback on this!

Regarding the README, i'd like to keep it short there and link to the Wiki, where I'm happy to provide more elaborate instructions on how to contribute. Hope I find motivation for that soon. :)

I'm definitely motivated to add more functionality to the library itself and improve the code structure overall. 🤞

@humanbydefinition
Copy link
Owner

Hey again, I decided to merge this to main already, since the most important things are done from my point of view.

As mentioned in the pull request, the Wiki entry regarding the Contributing Guidelines is still work-in-progress, as well as a more sophisticated workflow including proper tests.

I'll re-open this issue until then, also to receive potential feedback. :) I'm unsure if people are able to comment on a closed issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants