-
Notifications
You must be signed in to change notification settings - Fork 2
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
Added composite build settings.gradle file for running hoist inline #750
base: develop
Are you sure you want to change the base?
Conversation
I am very eager to get this merged - in fact I would like to use it more broadly across our apps at clients as well, where I am feeling the pain of not having wrapper projects setup to support development of client-specific plugins.
![]() @lbwexler are you good with this change? |
nevermind that's done sorry! |
client-app/package.json
Outdated
@@ -12,9 +12,11 @@ | |||
"lint:js": "eslint --ext .js,.jsx,.ts,.tsx .", | |||
"lint:styles": "stylelint \"**/*.s?(a|c)ss\"", | |||
"start": "yarn install && cross-env NODE_OPTIONS=--max_old_space_size=3072 webpack-dev-server", | |||
"start2": "yarn start --env devGrailsPort=8082 devWebpackPort=3002", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these updates doing? Does this version needs it own port- meaning will 8080 also be in use when running in this manner?
UPDATE: I see https://github.com/xh/toolbox/pull/750/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R111 - so this is for running multiple (two) client in parallel? For testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, indeed, this is for running a second client in development that will hit the second server
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update. Perhaps a name update to something like startSecondClient
/ startSecondClientWithHoist
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree the name 2 sounds like a potential replacement! Great catch. But I suggest just changing out the '2' with instance2
e.g. startInstance2
, `startInstance2WithHoist
Framework. A [toolbox-dev instance](https://toolbox-dev.xh.io) is auto-deployed via Teamcity on each | ||
commit to either the Toolbox or Hoist `develop` branches. We update a distinct | ||
["production" instance](https://toolbox.xh.io) manually with each new versioned Hoist release. | ||
Framework. A [toolbox-dev instance](https://toolbox-dev.xh.io) is auto-deployed via Teamcity on each commit to either the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not your change, but link Teamcity
to a remote source? Maybe most people know what this is, but I was only previously aware of Jenkins, Bamboo, Travis, CircleCI and AWS CodePipeLine. When I first read this I was unsure what Teamcity meant here.
### Editing multiple projects together | ||
|
||
For editing Hoist Core and React alongside Toolbox, it is recommended to open the `hoist-react` and | ||
`hoist-core` projects as modules in your editor. Having all three repos in a single IntelliJ project |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assume IJ for the IDE. Do we want to present this README with this assumption?
|
||
Note this is _only_ required if you're changing hoist-react code. | ||
|
||
### Running multiple instances of Toolbox |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point, I wonder if its worth standing up a Wiki or docs page for toolbox / hoist? If I just want to checkout toolbox to poke around, this doesn't come into play. Having this as an option in my startup commands is merited, but including it in the README may just confuse process of setting a basic setup running.
@@ -43,19 +43,10 @@ springBoot { | |||
mainClass = xhAppPackage + ".Application" | |||
} | |||
|
|||
if (parseBoolean(runHoistInline)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personal ignorance only - what does settings.gradle
do which we did not get here?
// Composite build setup for compiling hoist-core locally | ||
if (parseBoolean(runHoistInline)) { | ||
println "${xhAppName}: running with Hoist Core INLINE...." | ||
includeBuild '../hoist-core' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are including static pathing, we should 100% ensure the documentation denotes where things should live. It may, I just want to confirm we are thinking about this.
|
||
Note this is _only_ required if you're changing hoist-core code. | ||
|
||
* To run the client using the local `hoist-react`, you need `hoist-react` to exist as sibling of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An extension to what I have already said, but there is a lot of details here (and previously) about second tier setups (including hoist in local dev), but it maybe misses the quick, happy path setup.
I think this is ok for now, but it may be worth a think about how we present setup to a new user, and tier the information they receive - from quick start happy path to more in depth configurations as they need it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some (at least bare bones) info about the happy path above.
Overall: I left notes on the README updates after discussing with @jskupsik . My opinion overall through is not to bikeshed the README for this PR. I think it merits a bigger think about how we we want to present toolbox to somone visting the repo for the first time, and what we would want their "onboarding" experience to me. And this task is more something we would want to ticket out as tasks to look at as a company over time - adding documentation pages, wikis, online introduction videos, etc. |
Added a
settings.gradle
file to the toolbox app.It is capable of running toolbox as a normal hoist app, in addition to being able to run toolbox with local hoist using the gradle composite build (
includeBuild
).With this, we no longer need a wrapper directory containing toolbox and hoist-core (with an annoyingly specific gradle version installed), in order to run the app in inline mode.