-
-
Notifications
You must be signed in to change notification settings - Fork 46
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
base: master
Are you sure you want to change the base?
Conversation
@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 @@ | |||
/** |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why private?
There was a problem hiding this comment.
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.
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! |
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>
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. |
summary of changes:
uuid
dependency because it's very bloated, and it's not important to have cryptographically secure UUIDs for verlet simulationsimport
statements to be valid (e.g..,import Entity from './Entity';
is invalid because it lacks the.js
extensionpackage.json
:private
to avoid accidentally publishing it to npm until we're readyprepublishOnly
step which will ensure tests pass before publishing to npm (prep)AngleStick
parameters work