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

Proposal: switch to a real module system for better modular designs #245

Closed
kaosat-dev opened this issue Apr 21, 2017 · 27 comments
Closed

Comments

@kaosat-dev
Copy link
Contributor

kaosat-dev commented Apr 21, 2017

Curent system

Hi everyone ! right now we are using a custom and rather limited include system to enable modular design & part reuse, with a few major flaws:

pros

  • simple
  • custom

cons

  • reinventing the wheel: all the issues we face have already been solved elsewhere, better
  • global pollution : all variables declared in ANY include() are pushed to the global scope
  • unclear syntax for export & import : ie what is exported ?? what if we have mutlple functions with the same name in different modules etc (hint, not possible)
const someValue = 42
const someText = 'foo'
coolPart = function() { 
   return cube({size:someValue}).union(text(someText)
}
  • non idiomatic : the syntax teaches bad practices
  • no relative path resolution
  • greedy evaluation : any code in an imported module is immediatly evaluated & run (careful what you import!)
  • not integrated with the rest of the javascript ecosystem

Proposal

The node.js & js ecosystem have been very successfully using modules for years, let's use that tech and knowledge to our advantage :

Note: this applies to both common.js & es6 modules as well

pros

(in addition to all the cons of the current system, which are all resolved in commonjs/es6 modules)

  • clear , explicit and proven syntax :
    export: (coolPart.jscad)
const someValue = 42
const someText = 'foo' // this one is not exported, yet still available at the module level

function coolPart(){
  return cube({size:someValue}).union(text(someText)
}
module.exports = { //these are exported and available from any consumer
  someValue,
  coolPart
}

import

const {coolPart} = require('./coolPart.jscad') //we can selectively import features, etc
  • all the existing javascript packages & tools at our disposal
  • simpler to distribute package and use them (distribution with current syntax is not problematic, but use is)
  • OpenJSCAD is javascript, we should embrace it , instead of half heartedly hiding it :)

cons

  • higher complexity in browser (but less in node.js), there are many open source tools to code with modules in the browsers, they can be self hosted , but is more complex.

note: the commonjs require function / module loading is very simple and could also be emulated for a limited browser use

  • possible dependency issues (as in , too much)

Of course this could be rolled out progressively, tested & tried, and would not need to impact the current implementation of include.

As you can likely see, I am quite passionate about the subject .
Any thoughts ?

@z3dev
Copy link
Member

z3dev commented Apr 21, 2017

Actually, I think that the concept of include() came from OPENSCAD so possibly this compatibility can be maintained via the JSCAD library.

@kaosat-dev
Copy link
Contributor Author

@z3dev true, but not sure if it can /should behave the same way : unlike Openscad we have a full programming language at our disposal :)

@gfwilliams
Copy link

Just my 2p after submitting the metaball PR above...

This looks great (esp the common.js format for defining modules), but IMO simplicity (and it working easily on https://openjscad.org) is key.

The majority of your users won't be Node.js devs, so asking them to submit to NPM will probably cut down the amount of submissions you get a lot. The great bonus of openjscad.org is it works in the browser, so asking for a full Node.js install just to submit code seems like a step backwards.

While I mentioned require, chances are it wouldn't be that standards compliant with NPM and might just annoy people. include(url) might be preferable from that point of view.

Also, while I said about common.js above, having the ability to find someone's jscad object online and just include it into your design would be amazing. Perhaps the loader could check for the existence of the main function or exports fields and decide how to interpret file file based on that (or maybe just the file extension?).

@Spiritdude
Copy link
Contributor

Sorry for the late reply, I introduced include() as I wanted to keep things simple. In general OpenJsCad (the original) was OO-based and I wanted to add functions and UI to make it easier to use, openscad.js provided OpenSCAD-like JS functions, where include() was added in the same spirit.

Now that things we modular, it surely makes sense to adapt common ways, but keep include() as well. OpenJSCAD.org I did to be a hybrid: simple OpenSCAD-like, aside of the more elaborate OO approach which JS experts will use.

@gfwilliams
Copy link

Sorry, not sure how I missed that it's already there: https://en.wikibooks.org/wiki/OpenJSCAD_User_Guide#Including_Files

Looking at the docs, include via URL isn't possible yet... Would it be possible to add it? It'd be amazingly handy for referencing a file on GitHub, Gist or similar. Obviously cross-origin could be a pain, but I think GitHub at least is ok from that point of view?

@kaosat-dev
Copy link
Contributor Author

@gfwilliams
while I agree that simplicity is great and should be at the forefront , I don't thing it needs to be incompatible with a sane way to define & (re)use code:

  • just 'publishing' a javascript file with a package.json to github seems within reason (no need for npm for 'basic' use)
  • include() is reinventing the wheel and doing it really badly (I spent enough time trying to improve things in there for the past few months to have nightmares)
  • the semantics of include and mostly what and how you export it are horrible (globals !!) and frankly should be discouraged: even if people are new to Javascript when they start using jscad, we should not encourage broken design
  • security: loading remote scripts without a sandbox, and using our current eval-based approach is a big nono
  • what about the users that want both ? ie use it in the browser and in node (i am one of them , and not the only one :)
  • I think we can have a way to do both, without resorting to bad practices that will bite us down the line

Side notes:

  • this example shows that even if it really is a bit more complex, you can have stuff use node modules and also run in the browser & be modular https://esnextb.in/?gist=0a2ac2c4e189e27692ea964956a3a2e5
  • most of the interesting jscad designs I found where both on npm & github
  • I would argue that if you use code to create designs, you are at not allergic to tech :)

@kaosat-dev
Copy link
Contributor Author

@gfwilliams while adding loading from an URL is possible to add it relatively simply I really don't think it should be done : simplicity is good

  • but at the very worst we could have a node.js like require() running in the browser safely without having to resort to the include() hacks
  • it would work with the same semantics as common.js modules, so compatible with node.js for those who want it
  • I actually implemented something like that a while back , so I could do it again

@kaosat-dev
Copy link
Contributor Author

hi @Spiritdude ! :) I am ok with keeping include as it is, but building upon its very fragile foundations seem like a bad idea for all the reasons above.
side note , we only need functions, not OO haha (I have come to embrace functional like programming in JS, and man does it rock)

@kaosat-dev
Copy link
Contributor Author

btw I am also willing to provide a proof of concept for 'require-lite' functionality for demo purposes :)

@meta-meta
Copy link

I'm still VERY new to OpenJSCAD so I don't know the historic reasons for include() being the way it is and I'm not sure what the ultimate goals are for this project but I'm very interested in seeing a thriving open library of parametric CAD objects. Definitely opt for modules if possible. Having functions defined globally sounds totally insane to me as a front-end webapp developer with a functional programming bent. Coming in fresh, the OO-like function() main() { ... } had me wondering what magic parser was picking it up and what else was going on behind the scenes. It starts to feel like OpenJSCAD is a language extending javascript.

The wiki says

An OpenJSCAD script must have at least one function defined...

In es6 for say cube.js: export default () => { ... } gives you just that.
Then in some other module, import cube from './cube'

Essentially, I'll echo the "don't reinvent the wheel" arguments in this thread.

@meta-meta
Copy link

Check out https://aframe.io/aframe-registry/ for inspiration regarding a repo of community modules.

@kaosat-dev
Copy link
Contributor Author

thanks a lot for the feedback @meta-meta !
I also think modules is a must have : since we are planning a set of breaking API changes in the near future, it might make sense to include that as well.
The globals spamming IS horrible in every possible way :)

Wow a-frame has grown a lot since I last saw it ! Nice set of community modules!

Btw I have seen a lot of users create module based code that gets 'flattened' down (ie something like browserify) for simple online use, so I think it is up to us to making the modules approachable even for non technical folks ? Ie a simple UI that could wrap up your current work as a reuseable package? (with optional github/npm publishing ?)

I think we could re-use the engines field in package.json (just like Atom does, except we would set it to 'jscad' ) to be able to easily query jscad packages created by users... hmm lots of good stuff !

@kaosat-dev
Copy link
Contributor Author

As a follow up to this discussion, the newly relased pre alpha of jscad-desktop supports common.js code for jscad designs :)

https://github.com/jscad/jscad-desktop/blob/master/src/core/scripLoading.js#L75
loads either jscad code as common.js modules OR creates a virtual common.js module for jscad scripts which are not using it explicitly :)

I think there might be a way to do something very similar in the browser, stay tuned!

@kaosat-dev
Copy link
Contributor Author

A bit of update on this front :

  • modules are now supported in the CLI ( feat(modules support): add node.js modules support to cli #360 )
  • I am currently working on adding support for modules in the web UI as well (if used in 100% web mode it will be very limited, but when used with drag & drop of folders, it will be 100% compatible with node modules)

@udif
Copy link

udif commented Mar 12, 2018

Any feature that allows either of the 2 options below is welcomed!

  • specifying more than one JS file in a URL (one file contains the main() function, other are helper libraries)
  • specifying one file in the URL, but that file can require() from other files in the same location

This would finally allow giving out URLs of objects you have made that uses lower level libraries, without requiring you to copy/paste the libraries into a single file, or forcing the use of node.js for the same task.

@kaosat-dev
Copy link
Contributor Author

@udif while I can understand the appeal of those options, I am honestly not sure if they are feasible at all without major hassles: (as always, contributions are very welcome !)

  • for the first option : how would you pass more than one file, do you have an example ?
  • given the security restrictions, while that would be a nice option, I am not sure it could work (CORS & co).

If you see solutions for this that do not require a specific backend, please post , we are really open to discussing these things and finding solutions

about node.js : when I say node modules, I really mean the common.js syntax of require & imports : you could have a set of files containing / folders containing your different pieces of code without actually needing node.js itself : the main goal of the changes above is to move to a more sane management of reuseable code : it is no wonder pill that solves all issues, but at least we will stop doing hacky globals-insertions etc

@gfwilliams
Copy link

I do this kind of thing in the Espruino IDE - you can just do var baz = require("http://foo.bar/baz.js") if you want to (I know that's not very spec compliant, but it's really helpful).

CORS can bite you, but 99% of the time people are linking to GitHub, and in that case we detect the URL and just use GitHub's API to access the file directly.

@udif
Copy link

udif commented Mar 12, 2018

Before I start, let me just state that I have zero experience as a web frontend/backend designer. I merely use Openjscad and pure Javascript to create 3D objects.

@kaosat-dev regarding your points:

  1. We already have a syntax for passing a URL, what is the exact problem with extending this syntax for multiple URLs?
  2. Regrading the security issue: I am not a security expert, but I understand that security practices preclude a client webpage from loading javascript code from an arbitrary page (I have to accept the fact that more security-aware people had good reasons for this rule). What I dont understand is:
  • We already load one arbitrary javascript URL. Why adding more than one URL as an argument makes a fundamental change security-wise? (one is OK, two or more are not). I looked at the CORS specs and fail to see how the single URL load worked in the first place?!
  • We support multiple file by drag & drop. Why is this considered safe vs. encoding URLs in the URL query string?

I just saw @gfwilliams answer before I posted mine - limiting loading from github would be more than enough for me.

@udif
Copy link

udif commented Mar 12, 2018

I think I just answered myself:
It seems that www.openjscad.org is not merely a static web page but an actual server that can load these pages? My forked github.io copy of openjscad fails to load URIs :-(

@z3dev
Copy link
Member

z3dev commented Mar 12, 2018

Correct. There’s a small sever piece that happens behind the scene.

With the require() approach, this piece (and CORS) is not required.

@kaosat-dev
Copy link
Contributor Author

@gfwilliams that is very interesting ! I took a look at the relevant Espruino docs , it is a nice solution ! :)
Having specific adaptors (with API fallback) to 'popular websites' is very nice !
One thing I would very likely do differently for jscad though: not overriding require() , so it does not hide sync/async differences (that is a horrible part of the older jscad code for example).

Since dynamic import() is not really present yet in browsers I would likely just add mockImport()that returns a promise, making it distinct from sync require() calls. (yes, it adds another function to remember, but conflating things that work in a very different manner is already an issue with jscad, and if we ovveride require() with remote url support the scripts would not work in 'vanilla' node.js

@kaosat-dev
Copy link
Contributor Author

@udif it is a bit more complicated then that , sadly

  • the code for the things you mention actually needs to be written and work in a coherent manner:)
  • with multiple urls which is the main file, which is something else ? (it can be determined based on the functions inside the files but can be tricky)
  • as you found out & as @z3dev said, there is a small but essential server side component that actually downloads the remote files to the server , and passes them to JSCAD, circumventing the whole CORS issues) but it ain't always pretty, and ...it requires a server in the first place
  • we have very limited manpower for the project, and it never was a priority
  • drag & drop works with LOCAL files , from your hard drive, less security issues there (or different ones: but I am going to assume that if you use OpenJSCAD you trust both our site/code and your files :)

@gfwilliams
Copy link

In Espruino, for require we scan over the code first, pick out instances of require with a string and then load the modules in beforehand. It works in our case but it'd be far from ideal for you.

mockImport sounds like a great idea - at that point it's then basically just a function that you add into the library of functions that OpenJSCAD provides? Seems quite tidy - and I think it's fair to keep it in the browser and ignore the CORS issues? Pretty much everyone will be using it on some well known service like GitHub or on their own server where it's trivial to tweak the CORS headers.

@z3dev
Copy link
Member

z3dev commented Mar 13, 2018

Seems to me that include() and mockImport() are the same, unless you can point out something significantly different. Trying to provide something that looks like import() proposal... and let’s see...

  • dependency loading
  • asynchronous and synchronous loading
  • remote and local URL

Umm... nothing there is easy.

@kaosat-dev
Copy link
Contributor Author

kaosat-dev commented Mar 13, 2018

@gfwilliams thanks for the insights! pre-emptive loading works but can get a headache :) and adds complexity
And you are spot on with mockImport ! Small utility function, limited scope, syntax should follow https://github.com/tc39/proposal-dynamic-import as much as possible I think (still EAGERLY awaiting the day where we do not have to reinvent the wheel...).
Not sure if we should limit to web , as it would mean different scripts for browser & node, which should be avoided, but it could be a simple api function with a different implementation in web & node (or just trust browserify & co)

@z3dev they would not the same at all, one is sync, the other async : require() returns the exports of the given module, mockImport() would return a PROMISE of a module
import() itself will be added in the future to browsers & co https://github.com/tc39/proposal-dynamic-import

  • dependency loading : with CORS : yikes ! with api fallback like @gfwilliams would be feasible
  • async vs sync : hard, should be two different issues
  • remove and local URL: except for security issues, should be the same right ?

but yes, all this is horribly complex (too many edge cases): this is why for now , my main goal it jus to have support for modules , how/when/if to load them is another layer of complexity on top :)

(guess what? I managed to get node modules loading from the web drag & drop, and the crappy circular dependencies in CSG.js breaks stuff ... )

@z3dev
Copy link
Member

z3dev commented Mar 14, 2018

Cool! A little progress anyway.

CSG.js.... working on that little by little.

@z3dev
Copy link
Member

z3dev commented Apr 14, 2021

With the release of V2 JSCAD, this issue has been resolved as the Common.js (Node) method for require() has been adopted. This may be extended or changed in the future.

Thanks all for the feedback!

@z3dev z3dev closed this as completed Apr 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants