search

LaserWeb / LaserWeb4

Collaborative effort on the next version of LaserWeb / CNCWeb
GNU Affero General Public License v3.0
713 stars 192 forks source link

Fix build by installing all dependencies #568

Closed awhitty closed 5 years ago

awhitty commented 5 years ago

This PR is in reference to #562. It is a short-term fix, but I have more ideas for how to fix this in the long term.

I checked out the latest dev-es6 and found everything almost working! This PR has a few changes to make the build work.

I mentioned that I have some more ideas. The only reason the install.sh script is necessary is because the svg-parser package has been checked into this repo, and it has a few dependencies for its build. I don't have context on why that was done given that it's published as a separate package already, but I assume it was to incorporate a few patches into the package quickly.

I believe we can bring those patches back upstream into the lw.svg-parser repo, re-publish the package, and remove the checked-in version of the package from this repo. At that point, we should only need to interact with package as installed by npm and avoid installing dependencies inside that src/lib/lw.svg-parser folder.

cprezzi commented 5 years ago

I don't like the inconsistency of having some lw. modules checked into the package and others linked as submodules. Wouldn't it be better to attach all lw. modules as git submodules? What would be needed to change that? Just migrate the changes of the checked in version to the repo?

PS: I would not use separate shell scripts, because having a bash shell is not always true.

awhitty commented 5 years ago

I don't like the inconsistency of having some lw.* modules checked into the package and others linked as submodules.

I agree. It’s pretty funky and shouldn’t really be necessary here.

Wouldn't it be better to attach all lw.* modules as git submodules?

This is one solution, and another solution is to treat the package as any other node dependency and install through npm. I prefer the npm solution for the lw.svg-parser package because it’s being used like any other third party dependency.

The two submodules seem a little bit different to me. It looks like they’re being used to keep data separate from the rest of the repo. We could technically treat them as npm-installed packages just the same despite their unique concern, and that would eliminate most inconsistency there.

The final inconsistency I can find so far is the lw.raster2gcode folder. It appears related to the lw.raster-to-gcode repo, but the code looks very different! It’s not actively increasing complexity to get started in the repo, so I don’t think it needs any changing right now. We should probably reconcile the changes with its associated repo in the future though.

PS: I would not use separate shell scripts, because having a bash shell is not always true.

This is reasonable. The shell script shouldn’t be necessary if we iron out that svg-parser inconsistency, so I’ll focus on that instead.

Imo it’s very handy to have some pattern established for scripting in a repo to automate routine development tasks that can’t be inlined in the package.json. We could use js for those to keep things platform agnostic, but no motivation for it right now.

cprezzi commented 5 years ago

I do absolutely agree with you.

Just one hint: The is also a Travis compile job configured and there seems to be a problem with lw.svg-parser (related to babel preset). Do you have any experience with Travis?

awhitty commented 5 years ago

Closing in favor of https://github.com/LaserWeb/LaserWeb4/pull/573