search

comit-network / ambrosia

UI for trading in the COMIT network
Apache License 2.0
4 stars 3 forks source link

Pull in the comit-sdk #28

Closed da-kami closed 4 years ago

da-kami commented 4 years ago

Keep the SDK within the app until it is finished and then decide what to upstream. The app links the comit-sdk using the file: handle in the package.json.

Note: Initially pulled the SDK into the app folder, but when running yarn install in the project root (taker-ui folder) the comit-sdk was not properly installed inside app. By pulling it one level up (outside app) the installation of the dependencies work, however, changes on the comit-sdk are not hot-swapped into the app. (Which is annoying because running yarn install in the app folder is very time consuming). Note that initially I tried to pull the SDK code in without using it as a module (just link to the app-code directly and remove dependency from app's package.json), but that caused packaging errors that I could not easily resolve.

@thomaseizinger @bonomat if you have a better strategy for this once you see it please ping me. No need to invest too much brainpower though :)

da-kami commented 4 years ago

:( actually does not build & bundle properly like this. The comit-sdk install has to be triggered separately.

yosriady commented 4 years ago

:( actually does not build & bundle properly like this. The comit-sdk install has to be triggered separately.

Do you mean the yarn build command? Or the CI? Would adding a second app yarn install for the comit-sdk work in the Github Actions worfklow?

yosriady commented 4 years ago

To enable proper hot-swapping, I think we'd need to port the code to the app directly (importing the files and dependencies into the taker-ui project itself) instead of as a file module.

thomaseizinger commented 4 years ago

To enable proper hot-swapping, I think we'd need to port the code to the app directly (importing the files and dependencies into the taker-ui project itself) instead of as a file module.

Yep, that is what I suggested as well. I think @da-kami is now taking this approach :)

da-kami commented 4 years ago

Closing this for now, will re-open once sorted out.