zalo / CascadeStudio

A Full Live-Scripted CAD Kernel in the Browser
https://zalo.github.io/CascadeStudio/
MIT License
1.03k stars 129 forks source link

modules/webpack conversion. #58

Closed Irev-Dev closed 3 years ago

Irev-Dev commented 3 years ago

Converting CascadeStudio to use modules, bundled with Webpack.

Things that have changed:

1) There is already a package.json and a node_modules folder however modules in these folders were not really treated as ES modules (as in entire scripts were included into global space instead of importing specific functions into module scope much of the time) and were not managed by a package manager as they were under version control. With the exception of two "modules" all these files have been removed from version control. The two that remain in version control have been moved to a new folder called static_node_modules to delineate between the two (this is up for debate, they could also go into node_modules but I think separating makes sense. These two are opencascade.js since we're using our own fork, and rawflate.js (because I don't think exists on npm).

2) As a part of adding webpack I'v added number of build and dev tooling . . . things. Everything I added is fairly standard, but still somewhat opinionated since much of them have alternatives. I'll quickly break them down here. 2.1) .babelrc This package goes hand in hand with webpack, it's responsible for "transpiling" the code, and its what allows us to use modern js features and have still be compatible with old browsers, babel is overwhelming the default tool for transpiling code in webdev world. 2.2) .eslintrc.json I added eslint which is a style enforcing tool i.e. linting. It's opinionated to even have it part of a project since it's not needed but I generally agree with linting. in its current state, it doesn't do much besides allow us to specify rules (if you have eslint extension in vs or other editors it will pick up on rules and outline problems). In future we could set up commit cooks to format code for staged files. But again opinionated and I'm happy to remove. 2.3) postcss.config.js A css processor is needed to import css into js files. one of the main benefits of doing is that it allows the bundles to be split different pages in an SPA, so that only the css needed for a particular page is loaded. Not applicable for CascadeStudio atm since it's a single page and we can instead load directly in HTML. I guess it depends on what you think might happen with this project. A css processor also allows us to us sass syntax (nested selectors, mixins etc) if that of value to us. 2.4) I also added jest, jest is a test runner, there are a number of test runners, jest is just my favourite. Mocha would also be a solid choice. 2.5) I'v used a number webpack plugin and generally haven't explained the config too well, I'll leave more inline comments there. One other general webpack note is that you'll probably notice that the number of dev-dependencies has ballooned, that's kind of just the nature of using webpack.

3) index.html is no longer the main entry file, instead, it's now js/MainPage/index.js. It's standard practice for a bundled project, can't really use modules when loading scripts in HTML, one thing that can change though as mentioned above is removing the css from also being imported here, instead loading it in HTML will work the same, (it's easier doing so in js though since webpack is aware of it and will put the css into the dist/ build without us having to explicitly include in the webpack config.

4) Since we're using modules, the buttons have been changed from referencing global functions, they're are instead given unique ids which we add listeners to upon initialisation. https://github.com/zalo/CascadeStudio/pull/58/files#diff-734677b08737cfcfba04b154c9b8955d80d292748b3f1d8bc0de95007472fd8aR26

5) I've made an opinionated choice on how to handle the global variables that were in the project, The approach I choose was to optimize for a small diff. Good explanation here (https://github.com/zalo/CascadeStudio/pull/58/files#r562506897)

6) I had to make changes to have CacheOp works as arguments isn't available in modules/strict mode, it also required me to add a specific webpack setting to make sure function names were not minimized, more details in this comment. https://github.com/zalo/CascadeStudio/pull/58/files#r562879287

There are a number of comments inline where I've tried to explain some more decisions I've made.

vercel[bot] commented 3 years ago

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/zalo/cascade-studio/huptbsd9p
✅ Preview: https://cascade-studio-git-fork-irev-dev-kurt-webpack.zalo.vercel.app

Irev-Dev commented 3 years ago

Okay, I think this is ready for another look @zalo

So where's it at?

It's all bundled and all of the node_modules have been moved out of version control with the exception of opencascade.js (as we're using your fork) and deflate.js (as I don't think that exists on npm). It's successfully deployed to vercel (with the following settings) image

I'm working on doing regression testing atm.

Irev-Dev commented 3 years ago

For regression testing I've made sure that save and load project are working, all of the exports (step stl and obj) are working (though while the export of step "worked" I don't have any software that imports STEP files so wasn't able to check the shape output was okay beside the file exists and it's not 0 bytes. I'm not sure about importing shapes. I tried importing stl and it hangs on teststl.stl loaded successfully! Converting to OCC now... but so does the current live version at https://zalo.github.io/CascadeStudio, so I don't think that's a regression I've introduced (maybe it was just a bad stl file I was using 🤷 ).

In terms of evaling user code it renders the sample script just fine, I've also pasted all of the examples I currently have on Cadhub/your original gallery examples and it working with all of those.

image

image

image

image

image

image

image

image

image

image

Though it would probably a good idea for you to do your own testing.

Sorry for spamming you @zalo, I was getting this ready to ask for advice on fixing a caching issue I was having, but then I figured that out. So I think this is ready for a proper review (I'll remove [WIP]). Be as nitpicky as you like with any styles or whatever.

zalo commented 3 years ago

Apologies for taking so long to take a look at this; this is really interesting work! Just got it building/running locally.

It looks like it's necessary to use yarn to install over npm; what's the rationale here?
Should "install": "yarn install", be included in the package.json scripts?

Also, if it's not too much trouble, could you consolidate a brief summary of the changes this PR makes in the OP (now that it's mostly stable) so I can contextualize diffs in the PR more easily?

Also (2) that .stl loading bug is strange. I find that hitting the load STL button again in this PR causes it to unhang and load the STL, so it may be an asynchronous ordering/state bug... I do not see this behavior on the main branch.

Irev-Dev commented 3 years ago

Apologies for taking so long to take a look at this; this is really interesting work! Just got it building/running locally.

It looks like it's necessary to use yarn to install over npm; what's the rationale here? Should "install": "yarn install", be included in the package.json scripts?

There's a bit of history to the two tools, Yarn was a lot better when facebook released it, the npm cli tool has caught up somewhat, but yarn is still faster, but both are fine, really comes down to it being my preference. I'm happy to change it if you like, This is a good break down. https://www.javascriptwillrule.com/yarn-vs-npm-speed-tests

I wouldn't change the package.json scripts to include "install": "yarn install" seems like a pointless alias 🤷‍♂️

Also, if it's not too much trouble, could you consolidate a brief summary of the changes this PR makes in the OP (now that it's mostly stable) so I can contextualize diffs in the PR more easily?

I've had a go @zalo , happy to answer questions on anything though of course.

Also (2) that .stl loading bug is strange. I find that hitting the load STL button again in this PR causes it to unhang and load the STL, so it may be an asynchronous ordering/state bug... I do not see this behavior on the main branch.

Okay, I'll have another look at this one myself. It might have been a bad stl I was using.

Irev-Dev commented 3 years ago

The file load was a super simple fix, I just had the wrong listener on the input element, which is why it would work after the second click. 1eccb08

I had been putting it off because I thought it was going to be something harder to solve 😅

Any other concerns @zalo?

Irev-Dev commented 3 years ago

26196c4 is just updating with changes in master (upstream/master is the name of this repo's master branch in my fork)

Irev-Dev commented 3 years ago

Hey @zalo,

Just wanted to ask what's likely to happen with this PR? It's hard for me to gauge where on the spectrum from "intend to merge" to "undecided" to "rather not merge" it lies currently.

I'm conscious that part of my motivation to see this merged is because it will be better for CadHub (since Cadhub uses modules it makes the integration much better and easier), and so the last thing I would want to do is obligate you to do something that you don't think is in CascadeStudio's best interest. Having said that I do think that this is a good change for CascadeStudio too, because even with the added complexity of webpack, being able to use modules to organise code is a big win in the long term.

Part of the reason I'm asking now is because recently some of us have been talking about the direction of CadHub and we've put together a roadmap and we're kicking off work on another integration. So part of this work will see myself and @franknoirot doing our own implementation of the IDE-layout and probably the editor too, (i.e. having our own implementation of the editor will help us keep consistency between two or more integrations). The way we're thinking or hoping this will work with CascadeStudio might be something along the lines of taking the editor, the 3d-view and the console and putting each in their own modules in a way that they are completely unaware of any layout details, then we can have a forth module that slots them into golden-layout. From our perspective that will allow us to easily use a different layout tool, and or swap parts out (like perhaps the editor), but we're also hoping from your perspective that decoupling concerns in the app like this is also a good thing for CascadeStudio. That's somewhat theoretical since we haven't dug into the work yet, but the point is we'd like to submit more work to CascadeStudio along the lines of decoupling if you also think that the changes are mutually beneficial.

Sorry for the wall of text, the intention was to give lots of context as I didn't want this to come across as complaining that my PR hasn't been merged. My main concern is just getting some feedback as to what's likely to happen as that will help us plan.

I would also like to re-iterate that I'm more than happy to make more changes to this PR, or add more clarification to any areas (I've tried to add as much context as I could when I revamped the description, but it's likely I've got blind spots and missed areas), if jumping on a call would help with context, I'm very willing to do that too.

zalo commented 3 years ago

No, no; you’re right. It’s unfair for me to maintain radio-silence on this.

I’ve been thinking about it for a while, and my ultimate conclusion is that I love modules, but hate webpack.

I’ve been trying to find the motivation to come back and “harrow hell”, as it were, with browser-friendly/build-less ES6 modules, but I haven’t found it yet.

I don’t want to hold you back or for you to feel burdened with contributing back to this repo; so please take CadHub wherever you see fit! The OpenSCAD parsing you’re doing is really cool :-D

If I ever come back to CascadeStudio (for more than minor maintenance), it will likely be for some kind of VR-GUI (which would use code as a bidirectional backing serialization format).

Either way, I’d be happy to jump on a call to talk philosophy :-)

Irev-Dev commented 3 years ago

Cool it will be good to touch base.

VR sounds like a very interesting direction for the project 👨‍🚀.

I get that about webpack, I kinda see it as the lesser of evils. Modern web dev is a bit of a hot-mess, partially due to the nature of the platform and having backwards compatibility back to the 90's, though I'm hopeful it will be much better in half a decade.

Irev-Dev commented 3 years ago

Closing this as we've spoken about it and looks like webpack is not a good fit for CascadeStudio.

There's a chance esbuild is more suitable for this project and so this PR could be ported and re-opend, but there's no immediate plans for that.