-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add npm commands for dev and fix hotreloading #179
Conversation
targets/standalone/public/index.html
Outdated
@@ -1,65 +0,0 @@ | |||
<!DOCTYPE html> |
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.
Why did we remove this file?
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.
It was removed by mistake when updating its path, it is added back by the next PR https://github.com/Automattic/happychat-client/pull/180/files
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.
Ah, I see. You realize that if you add the file back in a different PR, you'll get the whole credit/blame? 😛
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.
Haha, I accidentally removed and added it back :) The file was moved from standalone/public/index.html
to standalone/index.html
. While commiting the removal here I used git commit -am
which did not add the new file. In the next PR I used git add --all
which added back the file that was on my machine (end of day PR splitting is fun)
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.
Hehe. :) So do you say that the file will keep its git history? If not, could you just re-add it back in this PR?
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 quite sure what you mean, after all sub branches are merged and everything will be squashed into master they all will be under one commit.
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.
It confuses me a bit that we delete and add files unrelated to the PRs goal, so I also prefer we add it back. But! I'm new to this mega-PR building trick, so I may be overly cautious.
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 agree this was removed by mistake and I accidentally added it in another PR when splitting into smaller PRs.
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.
Haven't tried all the commands but the code makes sense.
case 'development': | ||
config.devtool = 'source-map'; | ||
config.devServer = { | ||
contentBase: path.resolve( __dirname, 'targets/standalone' ), |
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.
❤️ we should merge this piece right away!
webpack.standalone.config.js
Outdated
case 'production': | ||
config.plugins.push( new webpack.optimize.ModuleConcatenationPlugin() ); | ||
config.plugins.push( new UglifyJsPlugin() ); | ||
config.plugins.push( new LodashModuleReplacementPlugin( { |
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.
We no longer use lodash-webpack-plugin
: it introduced subtle bugs and the size reduction it provided wasn't worth the hassle of changing our use of lodash to make it safe.
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.
Will remove it.
webpack.browser.config.js
Outdated
case 'production': | ||
config.plugins.push( new webpack.optimize.ModuleConcatenationPlugin() ); | ||
config.plugins.push( new UglifyJsPlugin() ); | ||
config.plugins.push( new LodashModuleReplacementPlugin( { |
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.
Same here: we no longer use lodash-webpack-plugin
.
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.
Will remove it.
webpack.wordpress.config.js
Outdated
case 'production': | ||
config.plugins.push( new webpack.optimize.ModuleConcatenationPlugin() ); | ||
config.plugins.push( new UglifyJsPlugin() ); | ||
config.plugins.push( new LodashModuleReplacementPlugin( { |
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.
Same here: we no longer use lodash-webpack-plugin
.
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.
Will remove it.
switch ( env ) { | ||
case 'development': | ||
config.devtool = 'source-map'; | ||
config.devServer = { |
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.
Do we use the development server in this case?
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 haven't actually tried it, but it should be helpful to serve local css while developing a wordpress plugin.
webpack.browser.config.js
Outdated
switch ( env ) { | ||
case 'development': | ||
config.devtool = 'source-map'; | ||
config.devServer = { |
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.
Do we use the development server in this case?
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 thought it might be useful, for whenever we try to embed HC client into a regular site to have a server that serves both js and css instead of publishing it somewhere.
module.exports = { | ||
const env = process.env.NODE_ENV; | ||
|
||
const config = { |
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 sure if I should mention this here as I've already mentioned it in the first PR: I'd prefer that targets were independent of each other. We should have a targets/wordpress/index.js
independent from targets/browser/index.js
.
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 you did, and this was fixed in the base PR.
package.json
Outdated
"dev:browser:css": "node-sass src/form.scss | postcss --config postcss.config.json -u autoprefixer -u postcss-custom-properties -o targets/browser/happychat.css", | ||
"dev:wordpress": "run-p dev:wordpress:*", | ||
"dev:wordpress:js": "NODE_ENV=development webpack-dev-server --config webpack.wordpress.config.js", | ||
"dev:wordpress:css": "node-sass src/form.scss | postcss --config postcss.config.json -u autoprefixer -u postcss-custom-properties -o targets/wordpress/assets/happychat.css", |
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.
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.
Are we using this local CSS files anywhere?
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.
While working on calypso integration I use the dev commands and it was very helpful to have local css served. Based on NODE_ENV the local css will be served by all dev:*
commands which tends to be extremely helpful while developing when css updates are needed.
Built on top of #178
Summary
Testing