-
Notifications
You must be signed in to change notification settings - Fork 35
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
Feature/multiple deployment #23
Feature/multiple deployment #23
Conversation
craUaAwareCodeSplitting="cra-ua-aware-code-splitting" | ||
reactMovieNetworkAwareComponents="react-movie-network-aware-components" | ||
reactShrineNetworkAwareCodeSplitting="react-shrine-network-aware-code-splitting" | ||
|
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.
Nothing to change with respect to this feedback, but leaving here for later:
In a typical review, I would normally say we should figure out how to define the list of subprojects in an array and iterate over the build and deploy steps for them in a loop as the steps are currently quite similar. That said, I actually like the flexibility we get with your current setup in case we need really granular control at some point.
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, I totally agree with you.
I was eager to use loop and array of projects because the current version looks not professional and even hardcoded.
But on the other hand I thought I should not spend much time on secondary problems.
So just for now I described this bash script to the meaningful extend because I thought I could get back to this refinement when I get some free or extra time.
TODO is remained here :)
cp -r $individualBuildAllFiles "${multipleBuildsPath}${craBatteryConsiderateLoading}" | ||
cd .. | ||
|
||
cd $craDeviceClassAwareLoading |
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.
Am I correct in saying if a build for a particular sub-project fails, we will bail out of building the others?
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.
I'm afraid I have not tried if we can skip the failure and keep going with the rest of builds.
Let me try by deliberately causing an error to one build.
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.
Based on what I've tested, we can skip the failure and keep going with the rest of builds.
@@ -1,6 +1,7 @@ | |||
{ | |||
"name": "memory-based-loading", | |||
"name": "cra-memory-considerate-loading", |
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.
These changes all LGTM. Thanks for aligning the naming around a common scheme.
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, thanks for your notice.
I like consistency very much. :)
I think I have to be flexible in working for saving time but fixing all minor issues by progressively improving the codebase whenever I notice them.
@@ -48,7 +48,7 @@ ReactDOM.render(( | |||
<ConnectedRouter history={history}> | |||
<ScrollToTop> | |||
<MuiThemeProvider theme={theme}> | |||
<Router> | |||
<Router basename="/react-shrine-network-aware-code-splitting"> |
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.
Is this a workaround for our deployment server requiring the full-path to be specified for routing to work at all with the current setup? If so, could we add in a comment above this line to note this?
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, this is a workaround but typical method for relative URL path.
https://facebook.github.io/create-react-app/docs/deployment
I'll add some comment about this line for sure.
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.
set basename prop for Building for Relative Paths from https://facebook.github.io/create-react-app/docs/deployment
I commented like above.
firebase-debug.log
Outdated
@@ -0,0 +1,66 @@ | |||
[debug] [2019-07-29T20:50:45.874Z] ---------------------------------------------------------------------- |
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.
Can we add firebase-debug.log to our gitignore?
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 I'll do that.
package.json
Outdated
"mkdirp": "^0.5.1", | ||
"string-similarity": "^3.0.0" | ||
} | ||
"license": "ISC" |
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.
To the extent possible, could we ensure all package.json licenses are set to Apache-2.0?
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, let me double check and write so.
No description provided.