From 2d4e5764f6cc1c40ee7362c19cf344b1256c7356 Mon Sep 17 00:00:00 2001 From: Nishant Mittal Date: Sun, 16 Aug 2020 08:54:52 +0530 Subject: [PATCH] Use faster devtools in webpack (#10228) * Devtools * Fixed backend tests * Fixed backend tests * Increase backend coverage * Source maps in lighthouse ci * Reviews * reviews * reviews * reviews * reviews --- .github/CODEOWNERS | 6 +- scripts/build.py | 15 +++++ scripts/build_test.py | 78 ++++++++++++++++++++++++++ scripts/release_scripts/deploy.py | 3 +- scripts/release_scripts/deploy_test.py | 11 +++- scripts/run_e2e_tests.py | 32 ++++++++--- scripts/run_e2e_tests_test.py | 46 ++++++++++++++- scripts/run_lighthouse_tests.py | 5 +- scripts/start.py | 13 ++++- webpack.dev.config.ts | 2 +- webpack.dev.sourcemap.config.ts | 25 +++++++++ webpack.prod.config.ts | 3 +- webpack.prod.sourcemap.config.ts | 25 +++++++++ 13 files changed, 239 insertions(+), 25 deletions(-) create mode 100644 webpack.dev.sourcemap.config.ts create mode 100644 webpack.prod.sourcemap.config.ts diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 16c62dbc850c..acc59a8a1510 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -560,11 +560,7 @@ /.lighthouserc.json @jimbyo @jameesjohn @vojtechjelinek /puppeteer-login-script.js @jimbyo /scripts/run_lighthouse_tests.py @jameesjohn @vojtechjelinek -/webpack.common.config.ts @jameesjohn @vojtechjelinek -/webpack.common.macros.ts @jameesjohn @vojtechjelinek -/webpack.dev.config.ts @jameesjohn @vojtechjelinek -/webpack.prod.config.ts @jameesjohn @vojtechjelinek -/webpack.terser.config.ts @nishantwrp @vojtechjelinek +/webpack.*.ts @nishantwrp @vojtechjelinek # Typings. diff --git a/scripts/build.py b/scripts/build.py index b442248c5b26..76e7b3dbcc4f 100644 --- a/scripts/build.py +++ b/scripts/build.py @@ -80,7 +80,10 @@ PARENT_DIR = os.path.abspath(os.path.join(os.getcwd(), os.pardir)) UGLIFY_FILE = os.path.join('node_modules', 'uglify-js', 'bin', 'uglifyjs') WEBPACK_FILE = os.path.join('node_modules', 'webpack', 'bin', 'webpack.js') +WEBPACK_DEV_CONFIG = 'webpack.dev.config.ts' +WEBPACK_DEV_SOURCE_MAPS_CONFIG = 'webpack.dev.soucemap.config.ts' WEBPACK_PROD_CONFIG = 'webpack.prod.config.ts' +WEBPACK_PROD_SOURCE_MAPS_CONFIG = 'webpack.prod.sourcemap.config.ts' WEBPACK_TERSER_CONFIG = 'webpack.terser.config.ts' # Files with these extensions shouldn't be moved to build directory. @@ -162,6 +165,12 @@ 'meaning that only super admins can access the site.' ) ) +_PARSER.add_argument( + '--source_maps', + action='store_true', + default=False, + dest='source_maps', + help='Build webpack with source maps.') def generate_app_yaml(deploy_mode=False, maintenance_mode=False): @@ -1331,7 +1340,13 @@ def main(args=None): minify_third_party_libs(THIRD_PARTY_GENERATED_DEV_DIR) hashes = generate_hashes() if options.deparallelize_terser: + if options.source_maps: + raise Exception( + 'source_maps flag shouldn\'t be used with ' + 'deparallelize_terser flag.') build_using_webpack(WEBPACK_TERSER_CONFIG) + elif options.source_maps: + build_using_webpack(WEBPACK_PROD_SOURCE_MAPS_CONFIG) else: build_using_webpack(WEBPACK_PROD_CONFIG) generate_app_yaml( diff --git a/scripts/build_test.py b/scripts/build_test.py index 8e255c82100a..9700359c7a74 100644 --- a/scripts/build_test.py +++ b/scripts/build_test.py @@ -991,6 +991,84 @@ def mock_compare_file_count(unused_first_dir, unused_second_dir): self.assertEqual(check_function_calls, expected_check_function_calls) + def test_build_with_prod_source_maps(self): + check_function_calls = { + 'build_using_webpack_gets_called': False, + 'ensure_files_exist_gets_called': False, + 'modify_constants_gets_called': False, + 'compare_file_count_gets_called': False + } + expected_check_function_calls = { + 'build_using_webpack_gets_called': True, + 'ensure_files_exist_gets_called': True, + 'modify_constants_gets_called': True, + 'compare_file_count_gets_called': True, + } + + expected_config_path = build.WEBPACK_PROD_SOURCE_MAPS_CONFIG + + def mock_build_using_webpack(config_path): + self.assertEqual(config_path, expected_config_path) + check_function_calls['build_using_webpack_gets_called'] = True + + def mock_ensure_files_exist(unused_filepaths): + check_function_calls['ensure_files_exist_gets_called'] = True + + def mock_modify_constants(prod_env, maintenance_mode): # pylint: disable=unused-argument + check_function_calls['modify_constants_gets_called'] = True + + def mock_compare_file_count(unused_first_dir, unused_second_dir): + check_function_calls['compare_file_count_gets_called'] = True + + ensure_files_exist_swap = self.swap( + build, '_ensure_files_exist', mock_ensure_files_exist) + build_using_webpack_swap = self.swap( + build, 'build_using_webpack', mock_build_using_webpack) + modify_constants_swap = self.swap( + build, 'modify_constants', mock_modify_constants) + compare_file_count_swap = self.swap( + build, '_compare_file_count', mock_compare_file_count) + + with ensure_files_exist_swap, build_using_webpack_swap: + with modify_constants_swap, compare_file_count_swap: + build.main(args=['--prod_env', '--source_maps']) + + self.assertEqual(check_function_calls, expected_check_function_calls) + + def test_cannot_build_with_source_maps_with_terser_config(self): + check_function_calls = { + 'ensure_files_exist_gets_called': False, + 'modify_constants_gets_called': False + } + expected_check_function_calls = { + 'ensure_files_exist_gets_called': True, + 'modify_constants_gets_called': True + } + + def mock_ensure_files_exist(unused_filepaths): + check_function_calls['ensure_files_exist_gets_called'] = True + + def mock_modify_constants(prod_env, maintenance_mode): # pylint: disable=unused-argument + check_function_calls['modify_constants_gets_called'] = True + + ensure_files_exist_swap = self.swap( + build, '_ensure_files_exist', mock_ensure_files_exist) + modify_constants_swap = self.swap( + build, 'modify_constants', mock_modify_constants) + assert_raises_regexp_context_manager = self.assertRaisesRegexp( + Exception, + 'source_maps flag shouldn\'t be used with ' + 'deparallelize_terser flag.') + + with ensure_files_exist_swap, modify_constants_swap: + with assert_raises_regexp_context_manager: + build.main( + args=[ + '--prod_env', '--source_maps', + '--deparallelize_terser']) + + self.assertEqual(check_function_calls, expected_check_function_calls) + def test_build_with_watcher(self): check_function_calls = { 'ensure_files_exist_gets_called': False, diff --git a/scripts/release_scripts/deploy.py b/scripts/release_scripts/deploy.py index 3f5932743d4c..ff6973ee797a 100644 --- a/scripts/release_scripts/deploy.py +++ b/scripts/release_scripts/deploy.py @@ -220,7 +220,8 @@ def build_scripts(maintenance_mode): # Do a build, while outputting to the terminal. python_utils.PRINT('Building and minifying scripts...') build_command = [ - 'python', '-m', 'scripts.build', '--prod_env', '--deploy_mode'] + 'python', '-m', 'scripts.build', '--prod_env', + '--source_maps', '--deploy_mode'] if maintenance_mode: build_command.append('--maintenance_mode') build_process = subprocess.Popen(build_command, stdout=subprocess.PIPE) diff --git a/scripts/release_scripts/deploy_test.py b/scripts/release_scripts/deploy_test.py index ba95b509082d..e5d116e42294 100644 --- a/scripts/release_scripts/deploy_test.py +++ b/scripts/release_scripts/deploy_test.py @@ -582,7 +582,9 @@ def mock_popen(tokens, stdout): # pylint: disable=unused-argument deploy.build_scripts(False) self.assertEqual( cmd_tokens, - ['python', '-m', 'scripts.build', '--prod_env', '--deploy_mode']) + [ + 'python', '-m', 'scripts.build', '--prod_env', + '--source_maps', '--deploy_mode']) def test_build_with_maintenance_mode(self): process = subprocess.Popen(['echo', 'test'], stdout=subprocess.PIPE) @@ -596,7 +598,8 @@ def mock_popen(tokens, stdout): # pylint: disable=unused-argument cmd_tokens, [ 'python', '-m', 'scripts.build', - '--prod_env', '--deploy_mode', '--maintenance_mode' + '--prod_env', '--source_maps', '--deploy_mode', + '--maintenance_mode' ] ) @@ -612,7 +615,9 @@ def mock_popen(tokens, stdout): # pylint: disable=unused-argument deploy.build_scripts(False) self.assertEqual( cmd_tokens, - ['python', '-m', 'scripts.build', '--prod_env', '--deploy_mode']) + [ + 'python', '-m', 'scripts.build', '--prod_env', + '--source_maps', '--deploy_mode']) def test_deploy_application(self): check_function_calls = { diff --git a/scripts/run_e2e_tests.py b/scripts/run_e2e_tests.py index 7c65517b9782..3e760fa7bb91 100644 --- a/scripts/run_e2e_tests.py +++ b/scripts/run_e2e_tests.py @@ -147,6 +147,11 @@ '--contributor_dashboard_enabled', action='store_true', help='Run the test after enabling the contributor dashboard page.') +_PARSER.add_argument( + '--source_maps', + help='Build webpack with source maps.', + action='store_true') + # This list contains the sub process triggered by this script. This includes # the oppia web server. SUBPROCESSES = [] @@ -204,15 +209,18 @@ def is_oppia_server_already_running(): return running -def run_webpack_compilation(): +def run_webpack_compilation(source_maps=False): """Runs webpack compilation.""" max_tries = 5 webpack_bundles_dir_name = 'webpack_bundles' for _ in python_utils.RANGE(max_tries): try: + webpack_config_file = ( + build.WEBPACK_DEV_SOURCE_MAPS_CONFIG if source_maps + else build.WEBPACK_DEV_CONFIG) subprocess.check_call([ common.NODE_BIN_PATH, WEBPACK_BIN_PATH, '--config', - 'webpack.dev.config.ts']) + webpack_config_file]) except subprocess.CalledProcessError as error: python_utils.PRINT(error.output) sys.exit(error.returncode) @@ -262,7 +270,8 @@ def setup_and_install_dependencies(skip_install): install_chrome_on_travis.main(args=[]) -def build_js_files(dev_mode_setting, deparallelize_terser=False): +def build_js_files( + dev_mode_setting, deparallelize_terser=False, source_maps=False): """Build the javascript files. Args: @@ -270,16 +279,22 @@ def build_js_files(dev_mode_setting, deparallelize_terser=False): in dev mode. deparallelize_terser: bool. Represents whether to use webpack compilation config that disables parallelism on terser plugin. + source_maps: bool. Represents whether to use source maps while + building webpack. """ if not dev_mode_setting: python_utils.PRINT(' Generating files for production mode...') + build_args = ['--prod_env'] + if deparallelize_terser: - build.main(args=['--prod_env', '--deparallelize_terser']) - else: - build.main(args=['--prod_env']) + build_args.append('--deparallelize_terser') + if source_maps: + build_args.append('--source_maps') + + build.main(args=build_args) else: build.main(args=[]) - run_webpack_compilation() + run_webpack_compilation(source_maps=source_maps) @contextlib.contextmanager @@ -543,7 +558,8 @@ def main(args=None): build.modify_constants(prod_env=parsed_args.prod_env) else: build_js_files( - dev_mode, deparallelize_terser=parsed_args.deparallelize_terser) + dev_mode, deparallelize_terser=parsed_args.deparallelize_terser, + source_maps=parsed_args.source_maps) version = parsed_args.chrome_driver_version or get_chrome_driver_version() python_utils.PRINT('\n\nCHROMEDRIVER VERSION: %s\n\n' % version) start_webdriver_manager(version) diff --git a/scripts/run_e2e_tests_test.py b/scripts/run_e2e_tests_test.py index 4f3e3be9a2b1..a24bd38df289 100644 --- a/scripts/run_e2e_tests_test.py +++ b/scripts/run_e2e_tests_test.py @@ -554,6 +554,43 @@ def test_build_js_files_in_prod_mode_with_deparallelize_terser(self): run_e2e_tests.build_js_files( False, deparallelize_terser=True) + def test_build_js_files_in_prod_mode_with_source_maps(self): + run_cmd_swap = self.swap_with_checks( + common, 'run_cmd', self.mock_run_cmd, called=False) + + build_main_swap = self.swap_with_checks( + build, 'main', self.mock_build_main, + expected_kwargs=[{'args': [ + '--prod_env', '--source_maps']}]) + + with self.constant_file_path_swap: + with self.node_bin_path_swap, self.webpack_bin_path_swap: + with build_main_swap, run_cmd_swap: + run_e2e_tests.build_js_files( + False, source_maps=True) + + def test_webpack_compilation_in_dev_mode_with_source_maps(self): + run_cmd_swap = self.swap_with_checks( + common, 'run_cmd', self.mock_run_cmd, called=False) + + build_main_swap = self.swap_with_checks( + build, 'main', self.mock_build_main, + expected_kwargs=[{'args': []}]) + + def mock_run_webpack_compilation(source_maps=False): + self.assertEqual(source_maps, True) + + run_webpack_compilation_swap = self.swap( + run_e2e_tests, 'run_webpack_compilation', + mock_run_webpack_compilation) + + with self.constant_file_path_swap: + with self.node_bin_path_swap, self.webpack_bin_path_swap: + with build_main_swap, run_cmd_swap: + with run_webpack_compilation_swap: + run_e2e_tests.build_js_files( + True, source_maps=True) + def test_tweak_webdriver_manager_on_x64_machine(self): def mock_is_windows(): @@ -802,7 +839,8 @@ def mock_register(unused_func, unused_arg=None): def mock_cleanup(): return - def mock_build_js_files(unused_arg, deparallelize_terser=False): # pylint: disable=unused-argument + def mock_build_js_files( + unused_arg, deparallelize_terser=False, source_maps=False): # pylint: disable=unused-argument return def mock_start_webdriver_manager(unused_arg): @@ -1083,7 +1121,8 @@ def mock_register(unused_func, unused_arg=None): def mock_cleanup(): return - def mock_build_js_files(unused_arg, deparallelize_terser=False): # pylint: disable=unused-argument + def mock_build_js_files( + unused_arg, deparallelize_terser=False, source_maps=False): # pylint: disable=unused-argument return def mock_start_webdriver_manager(unused_arg): @@ -1201,7 +1240,8 @@ def mock_register(unused_func, unused_arg=None): def mock_cleanup(): return - def mock_build_js_files(unused_arg, deparallelize_terser=False): # pylint: disable=unused-argument + def mock_build_js_files( + unused_arg, deparallelize_terser=False, source_maps=False): # pylint: disable=unused-argument return def mock_start_webdriver_manager(unused_arg): diff --git a/scripts/run_lighthouse_tests.py b/scripts/run_lighthouse_tests.py index 67506c5f6c66..36d1a95a0904 100644 --- a/scripts/run_lighthouse_tests.py +++ b/scripts/run_lighthouse_tests.py @@ -105,7 +105,10 @@ def main(): atexit.register(cleanup) python_utils.PRINT('Building files in production mode.') - build.main(args=['--prod_env']) + # We are using --source_maps here, so that we have at least one CI check + # that builds using source maps in prod env. This is to ensure that + # there are no issues while deploying oppia. + build.main(args=['--prod_env', '--source_maps']) build.modify_constants(prod_env=True) start_google_app_engine_server() common.wait_for_port_to_be_open(GOOGLE_APP_ENGINE_PORT) diff --git a/scripts/start.py b/scripts/start.py index 7d6cc866ceeb..fa4374435b0a 100644 --- a/scripts/start.py +++ b/scripts/start.py @@ -78,6 +78,11 @@ 'optional; if specified, does not automatically restart when files are ' 'changed.'), action='store_true') +_PARSER.add_argument( + '--source_maps', + help=( + 'optional; if specified, build webpack with source maps.'), + action='store_true') PORT_NUMBER_FOR_GAE_SERVER = 8181 @@ -122,6 +127,8 @@ def main(args=None): build_args = ['--prod_env'] if parsed_args.prod_env else [] if parsed_args.maintenance_mode: build_args.append('--maintenance_mode') + if parsed_args.source_maps: + build_args.append('--source_maps') build.main(args=build_args) app_yaml_filepath = 'app.yaml' if parsed_args.prod_env else 'app_dev.yaml' @@ -134,11 +141,15 @@ def main(args=None): if not parsed_args.prod_env: # In prod mode webpack is launched through scripts/build.py python_utils.PRINT('Compiling webpack...') + webpack_config_file = ( + build.WEBPACK_DEV_SOURCE_MAPS_CONFIG if parsed_args.source_maps + else build.WEBPACK_DEV_CONFIG) background_processes.append(subprocess.Popen([ common.NODE_BIN_PATH, os.path.join( common.NODE_MODULES_PATH, 'webpack', 'bin', 'webpack.js'), - '--config', 'webpack.dev.config.ts', '--watch'])) + '--config', webpack_config_file, '--watch'])) + # Give webpack few seconds to do the initial compilation. time.sleep(10) diff --git a/webpack.dev.config.ts b/webpack.dev.config.ts index 88715931806d..2a63821a1f88 100644 --- a/webpack.dev.config.ts +++ b/webpack.dev.config.ts @@ -26,7 +26,7 @@ module.exports = merge(common, { filename: '[name].bundle.js', path: path.resolve(__dirname, 'webpack_bundles') }, - devtool: 'inline-source-map', + devtool: 'eval', watchOptions: { aggregateTimeout: 500, poll: 1000 diff --git a/webpack.dev.sourcemap.config.ts b/webpack.dev.sourcemap.config.ts new file mode 100644 index 000000000000..cdbfb07035ca --- /dev/null +++ b/webpack.dev.sourcemap.config.ts @@ -0,0 +1,25 @@ +// Copyright 2020 The Oppia Authors. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS-IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +/** + * @fileoverview Development environment config file for Webpack with + * proper source maps. + */ + +const { merge } = require('webpack-merge'); +const dev = require('./webpack.dev.config.ts'); + +module.exports = merge(dev, { + devtool: 'inline-source-map' +}); diff --git a/webpack.prod.config.ts b/webpack.prod.config.ts index 2dc04e744b69..a33c1c6052b8 100644 --- a/webpack.prod.config.ts +++ b/webpack.prod.config.ts @@ -25,6 +25,5 @@ module.exports = merge(common, { output: { filename: '[name].[contenthash].bundle.js', path: path.resolve(__dirname, 'backend_prod_files/webpack_bundles') - }, - devtool: 'source-map' + } }); diff --git a/webpack.prod.sourcemap.config.ts b/webpack.prod.sourcemap.config.ts new file mode 100644 index 000000000000..6f3798c9ff7c --- /dev/null +++ b/webpack.prod.sourcemap.config.ts @@ -0,0 +1,25 @@ +// Copyright 2020 The Oppia Authors. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS-IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +/** + * @fileoverview Production environment config file for Webpack with + * proper source maps. + */ + +const { merge } = require('webpack-merge'); +const prod = require('./webpack.prod.config.ts'); + +module.exports = merge(prod, { + devtool: 'source-map' +});