-
Notifications
You must be signed in to change notification settings - Fork 74
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
[VsCoq1] Webpack Client and Server #266
Conversation
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.
Nice overall. Should have minimal conflicts with #276.
node_modules/.bin/vsce package | ||
|
||
clean: | ||
rm -rf node_modules out | ||
rm -rf node_modules client/out server/out html_views/out |
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.
This line can be replaced by a terse git clean -dfx
, if the .gitignore
is correct.
"package": "webpack --mode production --config webpack.config.js", | ||
"package:dev": "webpack --mode development --config webpack.config.js", | ||
"compile": "webpack --mode none --config webpack.config.js", | ||
"compile:client": "webpack --mode none --config webpack.client.config.js", | ||
"compile:html_views": "webpack --mode none --config ./webpack.html_views.config.js", | ||
"compile:server": "webpack --mode none --config ./webpack.server.config.js", | ||
"watch": "webpack --watch --mode none --config webpack.config.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.
I would drop "compile:server", "compile:client", and "compile:html_views", as webpack is smart enough to compile only what's necessary: a webpack
invoked from the toplevel directory should do whatever is necessary, and it's unlikely that any developer would invoke any of these three build targets individually.
Further, --config webpack.config.js
can be stripped, as webpack automatically looks for a webpack.config.js
by default. Perhaps add --devtool hidden-source-map
to "package", and drop the extraneous --mode none
, as the common config specifies this mode.
Finally, I might be tempted to drop the "package:dev" target, as it's unlikely to be used directly.
"compile:html_views": "webpack --mode none --config ./webpack.html_views.config.js", | ||
"compile:server": "webpack --mode none --config ./webpack.server.config.js", | ||
"watch": "webpack --watch --mode none --config webpack.config.js", | ||
"clean": "rimraf client/out && rimraf html_views/out && rimraf server/out" |
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.
This is duplicating code from the Makefile and unnecessarily adding a dependency.
//"outDir": "out", | ||
//"rootDir": "src", |
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.
Please strip these commented out lines.
"module": "commonjs", | ||
"target": "es6", |
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.
Consider factoring out these two lines into tsconfig.base.json
.
"lib": ["es6", "dom"], | ||
"sourceMap": true, |
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.
Consider factoring out these two lines too.
"module": "commonjs", | ||
"target": "es6", | ||
"outDir": "out", | ||
"tsBuildInfoFile": "out/tsconfig.tsBuildInfoFile", |
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.
Very nice!
//node: { | ||
// __dirname: false // leave the __dirname-behaviour intact | ||
//}, |
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.
Please remove this dead code.
@fakusb @Blaisorblade @huynhtrankhanh @4ever2. I'd like to include this to the next release. This is far beyond my competence, 😞 . Any ideas how to rebase this? |
@thery I rebased this and it seems to work okay after a few minor fixes. But still need to test it more. |
@4ever2 Thanks a lot. I think it is worth being merged. I will test it. |
This is superseded by #411 |
The client and server are now webpacked. To make this all work out, I had to split the single 'out' folder into three
out
folders, next to the already existing threesrc
folders. Also, now a few more files are ignored when bundling the extension as.vsix
file for publishing in the marketplaces (.vscodeignore
). And I could remove a dependency oncopy-webpack-plugin
, which more than halved the number of npm-packages we depend on.The debugging of the extension itself works as before, including breakpoints and sourcemaps.
The extension now loads ~20% faster (from a startup time of 100 ms to 80ms), and is smaller (312kB
.vsix
file instead of >5MB), and consists of a lot less files (The warning upon packaging the vsix file is now gone, so the nudging from microsoft to webpack worked for me).And when we want to support vscode-server, webpacking will be needed (so this is towards #257 ).
This superseeds #69.