Skip to content
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

Build improvements #2076

Merged
merged 18 commits into from
Jun 22, 2019
Merged

Conversation

lbogdan
Copy link
Contributor

@lbogdan lbogdan commented Jun 19, 2019

Description to come!

  • remove copy-webpack-plugin, replace it with
    • on dev, static serving, see
      const rootPath = path.resolve(__dirname, '../../..');
      staticAssets.forEach(({ from, to }) => {
      const fromPath = path.resolve(rootPath, from);
      devServer.use(`/${to}`, express.static(fromPath));
      });
    • on prod, copying the static assets to the build folder in the newly added app/scripts/copy-assets.js; also move all gulp build logic to this script
  • remove happypack, as it's in maintenance mode, and use thread-loader instead
  • update webpack to latest 4.35.0
  • update webpack-dev-server to 3.2.1 (newer versions fail in a weird way, should maybe look more into that - reminder )
  • refactor code related to displaying warnings and errors in start.js and build.js, show elapsed (re)build times
  • disable HMR
  • small package.json scripts changes

L.E.:

  • (temporarily?) remove the auto-complete branch from CodeMirror, because it's using tern, which has acorn as a dependency, which fails to build, see https://drone.ops.csb.dev/codesandbox/codesandbox-client/281
    we weren't loading the codemirror-tern chunk anyway, so I guess we were not using auto-complete? (although Ctrl-Space seems to work in the embeds?!)
Error in D:/work/webpack/codesandbox-client/node_modules/acorn/dist/acorn.es.js 3401:55
Module parse failed: Export 'parse_dammit' is not defined (3401:55)
You may need an appropriate loader to handle this file type, currently no loaders are configured to process this file. See https://webpack.js.org/concepts#loaders
| }
|
> export { version, parse, parseExpressionAt, tokenizer, parse_dammit, LooseParser, pluginsLoose, addLooseExports, Parser, plugins, defaultOptions, Position, SourceLocation, getLineInfo, Node, TokenType, tt as tokTypes, keywordTypes, TokContext, types as tokContexts, isIdentifierChar, isIdentifierStart, Token, isNewLine, lineBreak, lineBreakG };
 @ D:/work/webpack/codesandbox-client/node_modules/tern/lib/tern.js 10:15-31
 @ ./src/app/components/CodeEditor/CodeMirror/index.js
[...]

L.L.E.:

  • remove unneeded gulp tasks

L.L.L.E.:

Fixes #2069, #2070 .

@lbogdan lbogdan temporarily deployed to pr2076 June 19, 2019 13:24 Inactive
`Built ${stats.hasWarnings() ? 'with warnings ' : ''}in ${took /
1000}s.`
)
);
console.log();

// console.log('File sizes after gzip:');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can start showing this again after we remove CopyWebpackPlugin, I commented this because it was also showing all files copied by the plugin (+50,000 files), which made the build actually slow because of slow stdout.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can / should do something more involved as a separate step in the build process.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, like a check? I'd agree with that, if you can include it in this PR that would be nice, if not we could uncomment this for now in the meantime.

Copy link
Contributor Author

@lbogdan lbogdan Jun 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But how does that work, anyway, as in CI we don't have the previous build's assets?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do! We cache them between builds in CircleCI.

@lbogdan lbogdan temporarily deployed to pr2076 June 19, 2019 15:48 Inactive
@lbogdan lbogdan temporarily deployed to pr2076 June 19, 2019 17:16 Inactive
@lbogdan lbogdan temporarily deployed to pr2076 June 19, 2019 17:52 Inactive
@lbogdan lbogdan temporarily deployed to pr2076 June 19, 2019 18:07 Inactive
@lbogdan lbogdan temporarily deployed to pr2076 June 20, 2019 07:52 Inactive
@@ -63,6 +50,12 @@ const ESLINT_PLUGIN_VUE_INDEX = `module.exports = {

const sepRe = `\\${path.sep}`; // path separator regex

const threadPoolConfig = {
workers: 2,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did this make it faster than cpu().length - 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see any obvious improvements, even when using thread-loader vs. not using it, TBH. Can you maybe also test without it / with different numbers of workers and see what you get?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't see improvements either, let's keep it at 2.

@lbogdan lbogdan force-pushed the experiment/build-improvements branch from f3681a2 to 39fcae1 Compare June 20, 2019 15:01
@lbogdan lbogdan temporarily deployed to pr2076 June 20, 2019 15:09 Inactive
package.json Outdated
"**/react-codemirror2/**"
"**/react-codemirror2/**",
"app/webpack",
"app/acorn-dynamic-import"
Copy link
Contributor Author

@lbogdan lbogdan Jun 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Hacky?) Fix for webpack/webpack#8656 until acornjs/acorn#834 gets merged.

@lbogdan lbogdan temporarily deployed to pr2076 June 20, 2019 16:32 Inactive
@lbogdan lbogdan temporarily deployed to pr2076 June 21, 2019 05:41 Inactive
@lbogdan lbogdan changed the title [WIP] Build improvements Build improvements Jun 21, 2019
@lbogdan lbogdan marked this pull request as ready for review June 21, 2019 05:46
@@ -51,7 +54,7 @@ function formatMessage(message) {
function clearConsole() {
// This seems to work best on Windows and other systems.
// The intention is to clear the output so you can focus on most recent build.
process.stdout.write('\x1bc');
// process.stdout.write('\x1bc');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CompuIves Should we completely remove clearConsole()? I kind of get its purpose, but on Windows all the previous output is completely lost, which is frustrating when you'd want to look at previous warnings / errors.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I see. I liked the clearConsole but I think it makes sense to not do it in Windows. Maybe require('os').platform() === 'darwin' check?

Copy link
Contributor Author

@lbogdan lbogdan Jun 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disabled by default on Windows and added CLEAR environment variable to override default behavior.

@lbogdan lbogdan temporarily deployed to pr2076 June 21, 2019 06:16 Inactive
@lbogdan lbogdan temporarily deployed to pr2076 June 21, 2019 07:11 Inactive
@lbogdan
Copy link
Contributor Author

lbogdan commented Jun 21, 2019

Whooo, everything's back to green! 😎

// cm.toggleComment({ lineComment: '//' });
// });
// },
// 'Cmd-P': () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to enable this one? That will allow the file fuzzy search

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-added everything CodeMirror related back in.

@lbogdan lbogdan temporarily deployed to pr2076 June 22, 2019 09:44 Inactive
- use CLEAR environment variable to override default behavior.
@lbogdan lbogdan temporarily deployed to pr2076 June 22, 2019 10:20 Inactive
@CompuIves CompuIves merged commit 995a98a into codesandbox:master Jun 22, 2019
lbogdan added a commit that referenced this pull request Jun 24, 2019
* [WIP] Build improvements.

* Fix production build.

* Extract static assets to config/build.js and use them in both dev and prod scripts.

* Cleanup.

* Update yarn.lock.

* Update standalone-packages/vscode-textmate/package-lock.json.

* Bump CircleCI cache keys version.

* Revert "Update standalone-packages/vscode-textmate/package-lock.json."

* Small improvements.

* Update webpack to latest 4.35.0.

* CodeMirror: remove auto-complete, so we're not bundling tern.

* Remove unneeded gulp tasks.

* Update webpack-dev-server to latest 3.7.2 and remove unused webpack-dev-middleware.

* Show the updated module which triggers re-compiling.

* Don't use thread-loader while linting.

* Extract tern dep to codesandbox-deps and re-add CodeMirror bits.

* Disable console clearing on Windows by default and
- use CLEAR environment variable to override default behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Out of memory when run codesanbox at my PC
2 participants