-
Notifications
You must be signed in to change notification settings - Fork 26
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
Run closure compiler under webpack #135
Conversation
Does this mean |
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.
Any reason not to gitignore
the dist
directory?
webpack.config.js
Outdated
compiler: { | ||
jar: 'node_modules/google-closure-compiler/compiler.jar', | ||
compilation_level: 'SIMPLE', | ||
entry_point: 'BlocklyModule', |
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 you explain why we need BlocklyModule
and can't just have the entry point be Blockly
?
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.
The playground directly loads the rest of the uncompiled source files, but you can't use a JS file in the browser if it includes a module declaration, so I pulled that out into a separate file.
webpack.config.js
Outdated
blockly: path.join(__dirname, 'core/module.js'), | ||
}, | ||
output: { | ||
path: path.resolve(__dirname, 'dist'), |
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 there a specific reason we we join
for entry
and resolve
for output
?
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.
Nope.
tests/playground.html
Outdated
@@ -4,6 +4,7 @@ | |||
<meta charset="utf-8"> | |||
<title>Blockly Playground</title> | |||
<script type="text/javascript" src="../node_modules/google-closure-library/closure/goog/base.js"></script> | |||
<!--<script type="text/javascript" src="./test_dependency_map.js"></script>--> |
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.
nit: looks like a dev remnant
webpack.config.js
Outdated
|
||
module.exports = { | ||
name: 'blockly', | ||
mode: 'development', |
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 really want to default to dev and not prod?
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.
Fixed.
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.
couple nits, otherwise this is looking great!
package.json
Outdated
"build": "./deploy.sh", | ||
"build:dev": "./deploy.sh debug", | ||
"build": "webpack-cli", | ||
"build:dev": "DEV=1 webpack-cli", | ||
"preversion": "npm run test", | ||
"version": "npm run build:dev && npm run build && git add -A build-output", |
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.
nit: can remove the git add
step here
tests/playground.html
Outdated
@@ -5,6 +5,7 @@ | |||
<title>Blockly Playground</title> | |||
<script type="text/javascript" src="../node_modules/google-closure-library/closure/goog/base.js"></script> | |||
<script type="text/javascript" src="./test_dependency_map.js"></script> | |||
<script type="text/javascript" src="./test_dependency_map.js"></script> |
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.
looks like this is being included twice now? I think the last change should have been to remove this line rather than uncomment it
No description provided.