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

workspaces with vite for demo running, and rollup for module building #147

Merged
merged 12 commits into from
Nov 19, 2022

Conversation

oveddan
Copy link
Collaborator

@oveddan oveddan commented Nov 11, 2022

This PR breaks up the examples folder from the lib folder into separate workspaces, thus removing the dependency of three.js from the core lib and reducing its bundle size, and enabling examples to more freely import other dependencies that are not needed by the core lib, such as react, or @react-three/fiber.

Addresses #128

It takes inspiration from the workspace setup of react-three-fiber.

  • It uses preconstruct to handle the monorepo dev and code needs, including being able to locally link packages (in examples we can now do import { Vec3 } from behave-graph`
  • It builds the code with rollup and babel - this also generates nice source maps which i don't believe exist in the current build (I'm currently unable to step into code from the lib in chrome debugger when importing it from another app)
  • It runs the example using vite, a fast, modern, and well-documented front-end webdev environment.

Now, examples can import behave-graph code the 'normal' way (not by relative path), enabling the code to be copied and pasted more easily to those who want to do their own modifications on it:

Example code before:

import { Assert } from '../../lib/Diagnostics/Assert.js';
import { EventEmitter } from '../../lib/Events/EventEmitter.js';
import { IScene } from '../../lib/Profiles/Scene/Abstractions/IScene.js';
import { Vec3 } from '../../lib/Profiles/Scene/Values/Internal/Vec3.js';
import { Vec4 } from '../../lib/Profiles/Scene/Values/Internal/Vec4.js';

After:

import { Assert, EventEmitter, IScene, Vec3, Vec4 } from 'behave-graph';

Main organizational changes:

  • created /lib folder for the core package - maybe this can be eventually put into packaages/core
  • src/lib -> lib/src
  • src/graphs -> lib/graph`
  • src/examples -> examples/src`
  • added package.json to lib - this is the core lib package.json, and now behave-graph is the package name in there - should it be? Or should we do something like @behave-graph/core

Still todo:

  • fix export-node-spec script
  • figure out proper entrypoint for examples - right now there is an index.html in the root of examples that points to the three.js example
  • FIx the three example
  • update readme

@oveddan
Copy link
Collaborator Author

oveddan commented Nov 14, 2022

Do we need the export-node-spec script? I'm able to determine the node spec in my demos by calling: writeNodeSpecsToJSON(registry) to get the spec json - so not sure if that still needs to be part of the build step?

@@ -34,9 +34,9 @@
"watch": "preconstruct watch",
"start": "yarn workspace @behave-graph/examples start",
"build-examples": "yarn workspace @behave-graph/examples build",
"lint": "npx eslint \"src/**/*.{ts,json}\"",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! this was on my mental todo

@bhouston
Copy link
Owner

I did some more work in this branch here:

Currently issues:

  1. When I run "yarn exec-graph" in the root I get this error which seems that it is missing type definitions:
src/index.ts:17:8 - error TS7016: Could not find a declaration file for module '@behave-graph/core'. '/Users/bhouston/Work/behave-graph/lib/dist/behave-graph-core.cjs.js' implicitly has an 'any' type.
  Try `npm i --save-dev @types/behave-graph__core` if it exists or add a new declaration (.d.ts) file containing `declare module '@behave-graph/core';`

17 } from '@behave-graph/core';
          ~~~~~~~~~~~~~~~~~~~~
  1. when I run "yarn start" to run the three example I get this error:
Failed to load resource: the server responded with a status of 404 (Not Found)
Logger.ts:16 09:48:14 PM VERB:  reading behavior graph: http://localhost:5173/graphs/scene/actions/SpinningSuzanne.json
Graph.ts:61 Uncaught (in promise) Error: no registered node descriptions with the typeName math/create/euler
    at Graph.createNode (Graph.ts:61:13)
    at readNodeJSON (readGraphFromJSON.ts:130:22)
    at readGraphFromJSON (readGraphFromJSON.ts:48:5)
    at main (index.ts:84:17)

I know how to solve the second error. It was just me changing node type names -- which is a separate issue.

But the first issue with the exec-graph example not working, I am less sure how to fix.

Off to bed for this evening.

Moved them back into the `lib` folder, but now they are separate
entry points that can be called directly.  in reality export-node-spec
should not be called on its own
@oveddan
Copy link
Collaborator Author

oveddan commented Nov 15, 2022

Ok after a lot of trying of difference approaches, I've fixed this issue:

  • moving exec-graph and export-node-spec from examples into lib/src - and made them entrypoints so that they can be built as '.js' files and called directly.
  • updated the scripts to invoke those entry points.

I tried many things - the ideal solution would just to be able to have an exec-graph.ts node script that you can just build on its own, and have it import the behave-graph deps, but i could not get around the issue where importing esm modules. Also tried just running exec-graph/index.ts with ts-node - but it could not figure out how to get it to recognize .js extensions (which is a common issue with ts-node), which is how the lib imports other files. Just curious why not just have it import the file without the extension?

This seems like the best solution and it works as of now.

One issue - when running exec-graph from the root, it assumes that the base folder is within lib, so for example:
npm run exec-graph -- graphs/core/HelloWorld.json works, but yarn exec-graph -- lib/graphs/core/HelloWorld.json doesn't

iterations: string
};

function execGraph({
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a minor refactoring - in case we want to just export this function and call it directly from the outside world

"preconstruct": {
"entrypoints": [
"index.ts",
"exec-graph.ts",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 new entrypoints so that these scripts are built, and we can call them directly

@@ -42,6 +42,9 @@
"author": "",
"license": "ISC",
"dependencies": {
"commander": "^9.4.1",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these had to be moved into main dependencies for the build step to work

"export-node-spec": "npm run build && node --enable-source-maps ./dist/examples/export-node-spec/index.js"
"prepare": "yarn export-node-spec",
"exec-graph": "yarn workspace behave-graph exec-graph",
"export-node-spec": "yarn build && yarn workspace behave-graph export-node-spec"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

building where will build all the entrypoints within lib, allowing their individual 'dists' to be called

@oveddan oveddan changed the title WIP (do not merge) - workspaces with vite for demo running, and rollup for module building workspaces with vite for demo running, and rollup for module building Nov 15, 2022
fixed the three demo
@bhouston bhouston merged commit 3c73968 into bhouston:main Nov 19, 2022
@bhouston
Copy link
Owner

thank you! Merged in!

@oveddan
Copy link
Collaborator Author

oveddan commented Nov 19, 2022

awesome thank you!!!
I can create another one updating the readme

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

Successfully merging this pull request may close these issues.

2 participants