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

remove build step and uuid dependency #35

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mreinstein
Copy link
Contributor

@mreinstein mreinstein commented Feb 10, 2024

summary of changes:

  • remove the build step. no longer necessary
  • remove the uuid dependency because it's very bloated, and it's not important to have cryptographically secure UUIDs for verlet simulations
  • fix import statements to be valid (e.g.., import Entity from './Entity'; is invalid because it lacks the .js extension
  • clean up package.json:
    • update version number
    • make project name match the git repo name
    • set the the package to private to avoid accidentally publishing it to npm until we're ready
    • add a prepublishOnly step which will ensure tests pass before publishing to npm (prep)
  • visually document how the AngleStick parameters work

@mreinstein
Copy link
Contributor Author

@anuraghazra any thoughts on this PR? I see this one as a step before going all in on ES modules, and publishing to npm.

@@ -1,49 +0,0 @@
/**

Choose a reason for hiding this comment

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

Why delete this index.js file and put it straight into index.html? It goes against the principle of Separation of Concerns.

Copy link
Contributor Author

@mreinstein mreinstein Feb 12, 2024

Choose a reason for hiding this comment

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

I think there are a few reasons:

  • it's a demo file, intended to make it easy to follow along. Having to jump between less files makes for easier to read logic, which is more important than separation of concerns for a learning resource.
  • the file already isn't separating concerns. It was embedding style and html in several places in the same file.

"version": "1.1.3",
"name": "Verly.js",
"version": "1.3.0",
"private": true,

Choose a reason for hiding this comment

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

Why private?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it prevents publishing to npm. according to Anurag, we're not ready to do that yet. This is a temporary step to avoid accidental publish until we are ready.

@kungfooman
Copy link

I wouldn't bother deleting the entire build process, you can easily have ESM while keeping the build step alive. It may not be many people, but some people still just want a simple UMD to test this library in old browsers.

But anyway, converting this library to build-free ESM - nice!

@mreinstein
Copy link
Contributor Author

some people still just want a simple UMD to test this library

This library is quite easy to test, without a UMD bundle:

<script type="module">
import Verly from 'https://cdn.jsdelivr.net/gh/anuraghazra/Verly.js@1.4.0/src/Verly.js'

// ...
</script>

It may not be many people

I can appreciate the desire to be as inclusive as possible. Consider this:

My point being there are no Verly users that have touch events but lack esm.

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.

None yet

2 participants