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

Clarification on the use of the JavaScript modules #75

Open
pschroen opened this issue Jun 1, 2024 · 4 comments
Open

Clarification on the use of the JavaScript modules #75

pschroen opened this issue Jun 1, 2024 · 4 comments

Comments

@pschroen
Copy link
Contributor

pschroen commented Jun 1, 2024

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
Copy link
Owner

saharan commented Jun 4, 2024

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
Copy link
Contributor Author

pschroen commented Jun 4, 2024

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
Copy link
Contributor Author

pschroen commented Jun 4, 2024

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:

export const World = oimo.dynamics.World;

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
Copy link
Owner

saharan commented Jun 7, 2024

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.

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

No branches or pull requests

2 participants