saharan / OimoPhysics

A cross-platform 3D physics engine
MIT License
863 stars 68 forks source link

Clarification on the use of the JavaScript modules #75

Open pschroen opened 4 months ago

pschroen commented 4 months ago

Hey @saharan! 👋

So related to the previous TypeScript issue, I have a fix for that and can create a PR, but I'm unsure on the intended use of the index files in the bin directory.

They appear to be leftover from a three.js implementation by @mrEuler (@VBT-YTokan?), which has since been removed by three.js and has never been used by the package entry point itself, including the old version on npm.

Anyone who's been using this package in the past or currently through GitHub has been importing directly from the namespaces, for example:

import { oimo } from 'oimophysics';
console.log(new oimo.common.Vec3(0, -9.8, 0));

Or re-exporting the namespaces like in the index files, which allows the use of the library with a more traditional syntax:

import * as OIMO from 'oimophysics';
console.log(new OIMO.Vec3(0, -9.8, 0));
// or
import { Vec3 } from 'oimophysics';
console.log(new Vec3(0, -9.8, 0));

The index files are also missing some exports, and renaming some primitives like BoxGeometry to OBoxGeometry to prevent conflicts when used in other libraries like three.js.

It's also worth noting we don't need to rename the classes in the exports, a user who needs to import those classes can also rename the named import to whatever they want in their own app, for example:

import { BoxGeometry as OBoxGeometry } from 'oimophysics';

Anyways, all this to say, I'm fine using the library any of these ways, just wanted to clarify how you want to move forward with the index files, if they are meant to be the main entry point for the JavaScript package, or more as an example of how to re-export the namespaces for your own app?

For reference:

saharan commented 4 months ago

To be honest, I don't really know the point of those index files, which is mostly because I don't use node.js for my own. I think using import is a better and more flexible way, so I want to get rid of those files if they are not needed, even more so if they're causing troubles.

Also, thank you very much for tackling with #52! I plan to make a new npm package, will that resolve the issue or change its solution? It would be grateful if you or somebody else could give me some advice on making/moving a package on npm.

Thank you!

pschroen commented 4 months ago

No problem, to be clear the main entry point files, OimoPhysics.js and OimoPhysics.d.ts are both fine, the problem/confusion is only with the index files, which aren't being used and safe to delete. 👍

I'm happy to help with moving the npm package if needed, after the package is moved we should also update the README with the recommended use for JavaScript.

As for which syntax, for consistency with the non-module library files, I'm thinking we should use the following import syntax for the ES module as well:

import * as OIMO from 'oimophysics';
...

This is the same convention that three.js uses too.

pschroen commented 4 months ago

Actually, now that I'm taking a closer look at the non-module library files (I've never used them before), the namespaces are being exported by name there, for example:

window["OIMO"] = {};
window["OIMO"]["BroadPhase"] = oimo_collision_broadphase_BroadPhase;

This is different from the JS modules generated:

var oimo = oimo || {};
if(!oimo.collision) oimo.collision = {};
if(!oimo.collision.broadphase) oimo.collision.broadphase = {};
oimo.collision.broadphase.BroadPhase = class oimo_collision_broadphase_BroadPhase {

I think that was maybe the original intent of the index files, to export each class by name, but they are old and missing exports:

https://github.com/saharan/OimoPhysics/blob/68c1ba6395e9eaa6ac482a4387a5c55631008c3c/bin/js_modules/index.js#L4

Is there an option for Haxe to export the JS modules with the same exports that the non-module library files have?

For example, where the JS module file exports both BroadPhase and oimo_collision_broadphase_BroadPhase? That's essentially what the non-module library above is doing for comparison.

I still think we should delete the index files, this is more to have the JS modules work the same way.

saharan commented 4 months ago

I prefer the way the non-module library exposes its content too. Currently the module library keeps its nested namespace, which is because of the compiler option -D js_unflatten.

I tried removing Haxe's js_unflatten compiler option, but then it simply turns every single . before each type name to _, so oimo.collision.broadphase.BroadPhase will be oimo_collision_broadphase_BroadPhase and this is definitely not what we want.

I can insert a flattener function just before export {oimo} so that everything in oimo can be accessed like oimo.TypeName, but then OimoPhysics.d.ts will become inconsistent and fixing it is not that straightforward. I'm not sure if replacing oimo\.([a-z]+\.)* with oimo\. in OimoPhysics.d.ts will work.

I agree to delete the index files anyway.