modules/webpack conversion. #58
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/zalo/cascade-studio/huptbsd9p |
Irev-Dev
force-pushed
the
kurt/webpack
branch
from
72394b6 to
e363f81
Compare
| if ('serviceWorker' in navigator) { | ||
| navigator.serviceWorker.register('service-worker.js').then(function(registration) { | ||
| registration.update(); // Always update the registration for the latest assets | ||
| }, function() { | ||
| console.log('Could not register Cascade Studio for offline use!'); | ||
| }); | ||
| } else { | ||
| console.log('Browser does not support offline access!'); | ||
| } |
There was a problem hiding this comment.
This code has been moved to index.js which is the entry file for the js
| if (window.Worker) { | ||
| var cascadeStudioWorker = new Worker('./js/CADWorker/CascadeStudioMainWorker.js'); | ||
| // Ping Pong Messages Back and Forth based on their registration in messageHandlers | ||
| var messageHandlers = {}; | ||
| cascadeStudioWorker.onmessage = function (e) { | ||
| if(e.data.type in messageHandlers){ | ||
| let response = messageHandlers[e.data.type](e.data.payload); | ||
| if (response) { cascadeStudioWorker.postMessage({ "type": e.data.type, payload: response }) }; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Worker init code has been moved into its own file js/MainPage/CascadeWorkerInit.js, the worker is then imported into CascadeMain and CascadeView
| var preloadedFonts = ['../../fonts/Roboto.ttf', | ||
| '../../fonts/Papyrus.ttf', '../../fonts/Consolas.ttf']; | ||
| var fonts = {}; | ||
| preloadedFonts.forEach((fontURL) => { | ||
| opentype.load(fontURL, function (err, font) { | ||
| if (err) { console.log(err); } | ||
| let fontName = fontURL.split("./fonts/")[1].split(".ttf")[0]; | ||
| fonts[fontName] = font; | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Fonts are loading in js/CADWorker/CascadeStudioFontLoader.js and then imported into CascadeStudioStandardLibrary since that's the only place they're used.
| /** This function returns a version of the `inputArray` without the `objectToRemove`. */ | ||
| function Remove(inputArray, objectToRemove) { | ||
| return inputArray.filter((el) => { | ||
| return el.hash !== objectToRemove.hash || | ||
| el.ptr !== objectToRemove.ptr; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Since this was only ever used to Mutate sceneShapes I put it along with sceneShapes in their own module js/CADWorker/CascadeStudioSceneShapesService.js to be imported elsewhere, but I also changed the function a little, see comment there.
| export const resetSceneShapes = () => (sceneShapes = []); | ||
|
|
||
| /** This function returns a version of the `inputArray` without the `objectToRemove`. */ | ||
| export function RemoveFromSceneShapes(objectToRemove) { |
There was a problem hiding this comment.
No longer takes an array as an argument and instead assume you only want to mutate sceneShapes I can change it back? Is it likely that users will use this to remove shapes from their own array that aren't sceneShapes?
I also updated the IntelliSense file to suit.
| export const messageHandlers = {}; | ||
|
|
||
| export let argCache = {}; | ||
| export let currentLineNumber = 0; | ||
| export let currentOp = ""; | ||
| export let currentShape; | ||
| export let externalShapes = {}; | ||
| export let GUIState = {}; | ||
| export let oc = null; | ||
| export let opNumber = 0; // This keeps track of the progress of the evaluation | ||
| export let usedHashes = {}; | ||
|
|
||
| export const setArgCache = val => (argCache = val); | ||
| export const setCurrentOp = val => (currentOp = val); | ||
| export const setCurrentShape = val => (currentShape = val); | ||
| export const setCurrentLineNumber = val => (currentLineNumber = val); | ||
| export const resetExternalShapes = () => (externalShapes = {}); | ||
| export const setGUIState = val => (GUIState = val); | ||
| export const setOc = ocInit => (oc = ocInit); | ||
| export const setOpNumber = val => (opNumber = val); | ||
| export const setUsedHashes = val => (usedHashes = val); |
There was a problem hiding this comment.
A little background on modules and how vars are handled
(you probably already know but don't want to make assumptions).
Variables that are exported directly from modules can be used in other modules and are essentially getters for that variable in the original module. The variable can update, but only from within the module, and by update I mean re-assigned, of-course due to the way memory management works Arrays and Objects can be mutated outside of the original modules and will flow on to everywhere they are imported too.
So what have I done here?
Firstly my primary motive was optimize for the smallest diff in existing files as possible to lower the chance or severity of future conflicts (since this is a big change and there are other PR's currently up), This is apposed to optimizing for best practices or style. So in the case of global variables, I wanted export variables of the same name so everywhere in the code where these variables are read or mutated nothing would need to change, it's only in places where the variable needs to be re-assigned that the "setters" I've added here need to be used and will cause a diff.
That explains why I've chosen this particular method. The exact same explanation applies to js/MainPage/CascadeState.js. CascadeState is for the original main thread globals, and this file is for worker Globals.
There was a problem hiding this comment.
I appreciate the effort here; thank you.
| let fullShapeEdgeHashes = {}; | ||
| let fullShapeFaceHashes = {}; |
There was a problem hiding this comment.
I've left these here just to remind myself to ask about them, I think they can be removed. it's just this line made me think I need to be careful https://github.com/zalo/CascadeStudio/pull/58/files#diff-7d888ae3a42f72ffc3319a8a7a9476c873382cdfa729f59ef60b2c5ea257d44cL3
There was a problem hiding this comment.
I believe they're only used here:
https://github.com/zalo/CascadeStudio/blob/master/js/CADWorker/CascadeStudioShapeToMesh.js
Must be vestigial from when I was experimenting with different instanced-mesh, mouse-over/tooltip indexing techniques...
| @@ -534,14 +549,14 @@ const loadProject = async () => { | |||
| let fileSystemFile = await file.handle.getFile(); | |||
| let jsonContent = await fileSystemFile.text(); | |||
| window.history.replaceState({}, 'Cascade Studio','?'); | |||
| initialize(projectContent=jsonContent); | |||
| new initialize(jsonContent); | |||
There was a problem hiding this comment.
I was confused by this, as projectContent is not in scope. So I think I've done the right thing here.
| export const messageHandlers = {}; | ||
|
|
||
| export let monacoEditor = null; | ||
| export let threejsViewport = {}; | ||
| export let workerWorking = false; | ||
|
|
||
| export const setMonacoEditor = newEditor => (monacoEditor = newEditor); | ||
| export const setThreejsViewport = val => (threejsViewport = val); | ||
| export const setWorkerWorking = val => (workerWorking = val); |
There was a problem hiding this comment.
See this comment https://github.com/zalo/CascadeStudio/pull/58/files#r562506897
| return path; | ||
| } | ||
| }).then((openCascade) => { | ||
| initOpenCascade().then(openCascade => { |
There was a problem hiding this comment.
| /*"node_modules/three/build/three.d.ts", | ||
| "node_modules/three/build/three.min.js", | ||
| "node_modules/three/examples/js/controls/DragControls.js", | ||
| "node_modules/three/examples/js/controls/OrbitControls.js", | ||
| "node_modules/three/examples/js/controls/TransformControls.js", | ||
| "node_modules/three/examples/js/exporters/STLExporter.js", | ||
| "node_modules/three/examples/js/exporters/OBJExporter.js", | ||
| "node_modules/controlkit/bin/controlKit.js", | ||
| "node_modules/jquery/dist/jquery.min.js", | ||
| "node_modules/opentype.js/dist/opentype.min.js", | ||
| "node_modules/golden-layout/dist/goldenlayout.min.js", | ||
| "node_modules/golden-layout/src/css/goldenlayout-base.css", | ||
| "node_modules/golden-layout/src/css/goldenlayout-dark-theme.css", | ||
| "node_modules/rawflate/rawdeflate.js", | ||
| "node_modules/rawflate/rawinflate.js", | ||
| "node_modules/opencascade.js/dist/opencascade.wasm.js", | ||
| "node_modules/opencascade.js/dist/opencascade.wasm.wasm", | ||
| "node_modules/monaco-editor/min/vs/editor/editor.main.css", | ||
| "node_modules/monaco-editor/min/vs/loader.js", | ||
| "node_modules/monaco-editor/min/vs/editor/editor.main.nls.js", | ||
| "node_modules/monaco-editor/min/vs/editor/editor.main.js", | ||
| "node_modules/monaco-editor/min/vs/basic-languages/javascript/javascript.js", | ||
| "node_modules/monaco-editor/min/vs/basic-languages/typescript/typescript.js", | ||
| "node_modules/monaco-editor/min/vs/language/typescript/tsMode.js", | ||
| "node_modules/monaco-editor/min/vs/language/typescript/tsWorker.js", | ||
| "node_modules/monaco-editor/min/vs/base/worker/workerMain.js", | ||
| "node_modules/monaco-editor/min/vs/base/browser/ui/codiconLabel/codicon/codicon.ttf", | ||
| "node_modules/opencascade.js/dist/oc.d.ts", | ||
| /*"static_node_modules/rawflate/rawdeflate.js", | ||
| "static_node_modules/rawflate/rawinflate.js", | ||
| "static_node_modules/opencascade.js/dist/opencascade.wasm.js", | ||
| "static_node_modules/opencascade.js/dist/opencascade.wasm.wasm", | ||
| "static_node_modules/opencascade.js/dist/oc.d.ts", |
There was a problem hiding this comment.
Because I removed a number of the packages from static_node_modules (was node_modules), and now they are bundled, even though these are commented out I figured it was best to clean them up since they won't be cached in future.
| test('adds 1 + 2 to equal 3', () => { | ||
| expect(1+2).toBe(3); | ||
| }); |
There was a problem hiding this comment.
Only related to this PR because it comes under the umbrella of dev tooling, I thought it would be good to have tests ready to go, so this works with yarn test
| @@ -221,4 +221,4 @@ function CacheOp(arguments: IArguments, cacheMiss: () => oc.TopoDS_Shape): oc.To | |||
| /** Remove this object from this array. Useful for preventing objects being added to `sceneShapes` (in cached functions). | |||
| * [Source](https://github.com/zalo/CascadeStudio/blob/master/js/CADWorker/CascadeStudioStandardLibrary.js) | |||
| * @example```let box = CacheOp(arguments, () => { let box = Box(x,y,z); sceneShapes = Remove(sceneShapes, box); return box; });``` */ | |||
| function Remove(array: any[], toRemove: any): any[]; | |||
| function RemoveFromSceneShapes(toRemove: any): any[]; | |||
There was a problem hiding this comment.
some of the current files in node_modules will be moved to static_node_modules so they can continue to be used
Irev-Dev
force-pushed
the
kurt/webpack
branch
from
e363f81 to
ef2a708
Compare
Irev-Dev
changed the title
Irev-Dev
force-pushed
the
kurt/webpack
branch
from
1a52cfb to
a7a005b
Compare
|
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) I'm working on doing regression testing atm. |
|
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. 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.
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. |
Irev-Dev
changed the title
| from: "js/StandardLibraryIntellisense.ts", | ||
| to: "js/StandardLibraryIntellisense.ts" | ||
| }, | ||
| { | ||
| from: "static_node_modules/opencascade.js/dist/oc.d.ts", | ||
| to: "opencascade.d.ts" | ||
| }, | ||
| { | ||
| from: "node_modules/three/src/Three.d.ts", | ||
| to: "Three.d.ts" |
There was a problem hiding this comment.
Making these three definition files available as assets for the web-app to grab later, an alternative to this would be another rule that matches on these specific files to import them as text (instead of as code) so that they would be added to the main bundle instead of needing to be fetched later.
js/MainPage/CascadeMain.js
Outdated
| fetch(prefix + "/js/StandardLibraryIntellisense.ts").then((response) => { | ||
| response.text().then(function (text) { | ||
| extraLibs.push({ content: text, filePath: 'file://' + prefix + '/js/StandardLibraryIntellisense.d.ts' }); | ||
| const typescriptDefinitionFiles = ["opencascade.d.ts", "Three.d.ts", "js/StandardLibraryIntellisense.ts"] |
There was a problem hiding this comment.
See this comment for where these strings come from. https://github.com/zalo/CascadeStudio/pull/58/files#r563039339
Which is need to show which operation is being used in the console. Alternatively we could pass in a string instead of a reference to the function itself.
|
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 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. |
| }, | ||
| plugins: [ | ||
| new HtmlWebpackPlugin({ | ||
| template: path.resolve(__dirname, "index.html") |
There was a problem hiding this comment.
Because webpack primarily works for js files, this plugin is basically here to tell it to put index.html into the dist folder (the built folder that will be exposed to the web) and it adds a script tag to load our entry index.js file.
There was a problem hiding this comment.
I can see the confused emoji @zalo, I'll try to clarify but if I miss the mark let me know what you confused by and I might be able to provide better context.
If you build the production files by yarn build-prod it will put a bunch of files into /dist (and that's what vercel will serve up for us).
There are a bunch files in there, so runtime.bundle[hash].js is the main bundle (hashes are added for cache invalidation reasons I believe), the hash makes it difficult for us to include the bundle manually though, so letting webpack handle putting index.html into dist means it can include this file, so if you look at index.html in /dist folder, you see a script tag for the runtime bundle.
| template: path.resolve(__dirname, "index.html") | ||
| }), | ||
| new ServiceWorkerWebpackPlugin({ | ||
| entry: path.join(__dirname, 'service-worker.js'), |
There was a problem hiding this comment.
This plugin handles making a separate service worker bundle and makes it easy to include by importing the plugin's runtime
| new ServiceWorkerWebpackPlugin({ | ||
| entry: path.join(__dirname, 'service-worker.js'), | ||
| }), | ||
| new MonacoWebpackPlugin(), |
There was a problem hiding this comment.
Makes importing the Monaco editor easy.
| entry: path.join(__dirname, 'service-worker.js'), | ||
| }), | ||
| new MonacoWebpackPlugin(), | ||
| new CopyPlugin({ |
There was a problem hiding this comment.
This is just for copying files to dist as part of the build, so that they're available to be fetched from the app..
| optimization: { | ||
| runtimeChunk: "single", | ||
| splitChunks: { | ||
| cacheGroups: { |
There was a problem hiding this comment.
These are just basic configuring how the build is optimised, this cache group is basically any third party code should be it's own bundle and be cached, since code in node_modules is going to change less often than CascadeStudio code.
| minimizer: [ | ||
| new TerserPlugin({ | ||
| terserOptions: { | ||
| keep_fnames: true |
There was a problem hiding this comment.
See comment about CacheOp as to why this is needed.
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
I've had a go @zalo , happy to answer questions on anything though of course.
Okay, I'll have another look at this one myself. It might have been a bad stl I was using. |
| { | ||
| test: /opencascade\.wasm\.wasm$/, | ||
| type: "javascript/auto", | ||
| loader: "file-loader" |
There was a problem hiding this comment.
This recommended in the opencascade readme
|
26196c4 is just updating with changes in master (upstream/master is the name of this repo's master branch in my fork) |
|
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 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. |
|
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 :-) |
|
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. |
|
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. |
Converting CascadeStudio to use modules, bundled with Webpack.
Things that have changed:
There is already a package.json and a
node_modulesfolder 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 calledstatic_node_modulesto delineate between the two (this is up for debate, they could also go intonode_modulesbut 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).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)
.babelrcThis 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.jsonI 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.jsA 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.
index.htmlis no longer the main entry file, instead, it's nowjs/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.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
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)
I had to make changes to have
CacheOpworks asargumentsisn'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#r562879287There are a number of comments inline where I've tried to explain some more decisions I've made.