From ad7f11411cdca825e2e9093554b0fa9fda85783d Mon Sep 17 00:00:00 2001 From: Jared McDonald Date: Sat, 8 Oct 2016 12:54:44 -0400 Subject: [PATCH 01/12] set up modifyBabelConfig in base config and add it to buildConfigs for webpack --- utils/buildConfigs.js | 18 +++++++++++++++++- utils/kytConfig.js | 2 +- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/utils/buildConfigs.js b/utils/buildConfigs.js index 49d7d918a..d229149dd 100644 --- a/utils/buildConfigs.js +++ b/utils/buildConfigs.js @@ -47,7 +47,7 @@ module.exports = (config, environment = 'development') => { clientConfig = merge.smart(baseConfig(clientOptions), clientConfig(clientOptions)); serverConfig = merge.smart(baseConfig(serverOptions), serverConfig(serverOptions)); - // Modify via userland config + // Modify via userland webpack config try { clientConfig = config.modifyWebpackConfig(clone(clientConfig), clientOptions); serverConfig = config.modifyWebpackConfig(clone(serverConfig), serverOptions); @@ -56,6 +56,22 @@ module.exports = (config, environment = 'development') => { process.exit(1); } + // Merge in userland babel config via `config.transformBabelConfig` + try { + [ + { webpackConfig: clientConfig, options: clientOptions }, + { webpackConfig: serverConfig, options: serverOptions }, + ].forEach(({ webpackConfig, options }) => { + const babelLoader = webpackConfig.module.loaders.find(({ loader }) => loader === 'babel-loader'); + Object.assign(babelLoader, { + query: config.modifyBabelConfig(clone(babelLoader.query), options), + }); + }); + } catch (error) { + logger.error('Error in your kyt.config.js modifyBabelConfig():', error); + process.exit(1); + } + if (config.debug) { logger.debug('Client webpack configuration:', clientConfig); logger.debug('\n\n'); diff --git a/utils/kytConfig.js b/utils/kytConfig.js index 13633061e..9a0ebd940 100644 --- a/utils/kytConfig.js +++ b/utils/kytConfig.js @@ -40,7 +40,7 @@ module.exports = (optionalConfig) => { config = mergeAll([{}, baseConfig, config]); // Create default identity functions for modify functions - ['modifyWebpackConfig', 'modifyJestConfig'].forEach((m) => { + ['modifyWebpackConfig', 'modifyJestConfig', 'modifyBabelConfig'].forEach((m) => { if (typeof config[m] !== 'function') { config[m] = c => c; } From 931af3b7da166da75e62af92b65adb9c2c3ab639 Mon Sep 17 00:00:00 2001 From: Jared McDonald Date: Sat, 8 Oct 2016 13:00:54 -0400 Subject: [PATCH 02/12] use modifyBabelConfig in jest preprocessor --- utils/jest/preprocessor.js | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/utils/jest/preprocessor.js b/utils/jest/preprocessor.js index bb2a4b9a8..7f33f6fb6 100644 --- a/utils/jest/preprocessor.js +++ b/utils/jest/preprocessor.js @@ -1,17 +1,7 @@ -const mergeAll = require('ramda').mergeAll; const babel = require('../../config/babel')(); const babelJest = require('babel-jest'); -const buildConfigs = require('../buildConfigs'); const config = require('../kytConfig')(); -const { clientConfig } = buildConfigs(config); - -// Merge userland babel config with our babel config -// This should go away after https://github.com/NYTimes/kyt/issues/134 -const clientBabelConfig = clientConfig.module.loaders - .find(loader => loader.loader === 'babel-loader') - .query; - -const babelConfigForJest = mergeAll([{}, babel, clientBabelConfig]); +const babelConfigForJest = config.modifyBabelConfig(babel); module.exports = babelJest.createTransformer(babelConfigForJest); From 350c1a00ae60b649b2ccbfe04b620de2e6423af1 Mon Sep 17 00:00:00 2001 From: Jared McDonald Date: Sat, 8 Oct 2016 13:47:15 -0400 Subject: [PATCH 03/12] move call to modifyBabelConfig to config/babel instead of tying it to webpack config assembly; get tests to pass --- config/__tests__/babel.test.js | 2 ++ config/babel.js | 11 ++++++++++- utils/__mocks__/kytConfig.js | 4 ++++ utils/buildConfigs.js | 16 ---------------- utils/jest/preprocessor.js | 7 ++++++- 5 files changed, 22 insertions(+), 18 deletions(-) create mode 100644 utils/__mocks__/kytConfig.js diff --git a/config/__tests__/babel.test.js b/config/__tests__/babel.test.js index 774f5bae2..5113c98d7 100644 --- a/config/__tests__/babel.test.js +++ b/config/__tests__/babel.test.js @@ -15,6 +15,8 @@ jest.setMock('path', { resolve: a => a, }); +jest.mock('../../utils/kytConfig'); + const babel = require('../babel'); describe('babel', () => { diff --git a/config/babel.js b/config/babel.js index 037959d5e..6ebb8adc1 100644 --- a/config/babel.js +++ b/config/babel.js @@ -1,5 +1,7 @@ const path = require('path'); const fs = require('fs'); +const config = require('../utils/kytConfig')(); +const logger = require('../cli/logger'); // Uses require.resolve to add the full paths to all of the plugins // and presets, making sure that we handle the new array syntax. @@ -26,5 +28,12 @@ module.exports = (options) => { } Object.keys(babelrc.env || {}).forEach(env => resolvePluginsPresets(babelrc.env[env])); - return babelrc; + // modify via userland kytConfig's `modifyBabelConfig` + try { + return config.modifyBabelConfig(babelrc, options); + } catch (error) { + logger.error('Error in your kyt.config.js modifyBabelConfig():', error); + // return to satisfy eslint `consistent-return` rule + return process.exit(1); + } }; diff --git a/utils/__mocks__/kytConfig.js b/utils/__mocks__/kytConfig.js new file mode 100644 index 000000000..b820d7177 --- /dev/null +++ b/utils/__mocks__/kytConfig.js @@ -0,0 +1,4 @@ +module.exports = () => ({ + // TODO add more to this mock (this is all we need for now) + modifyBabelConfig: jest.fn(c => c), +}); diff --git a/utils/buildConfigs.js b/utils/buildConfigs.js index d229149dd..fdf958b97 100644 --- a/utils/buildConfigs.js +++ b/utils/buildConfigs.js @@ -56,22 +56,6 @@ module.exports = (config, environment = 'development') => { process.exit(1); } - // Merge in userland babel config via `config.transformBabelConfig` - try { - [ - { webpackConfig: clientConfig, options: clientOptions }, - { webpackConfig: serverConfig, options: serverOptions }, - ].forEach(({ webpackConfig, options }) => { - const babelLoader = webpackConfig.module.loaders.find(({ loader }) => loader === 'babel-loader'); - Object.assign(babelLoader, { - query: config.modifyBabelConfig(clone(babelLoader.query), options), - }); - }); - } catch (error) { - logger.error('Error in your kyt.config.js modifyBabelConfig():', error); - process.exit(1); - } - if (config.debug) { logger.debug('Client webpack configuration:', clientConfig); logger.debug('\n\n'); diff --git a/utils/jest/preprocessor.js b/utils/jest/preprocessor.js index 7f33f6fb6..700e09f99 100644 --- a/utils/jest/preprocessor.js +++ b/utils/jest/preprocessor.js @@ -2,6 +2,11 @@ const babel = require('../../config/babel')(); const babelJest = require('babel-jest'); const config = require('../kytConfig')(); -const babelConfigForJest = config.modifyBabelConfig(babel); +const options = { + type: 'test', + environment: 'test', +}; + +const babelConfigForJest = config.modifyBabelConfig(babel, options); module.exports = babelJest.createTransformer(babelConfigForJest); From 00ffded1289b20083dfd9f44adb01676d334bc36 Mon Sep 17 00:00:00 2001 From: Jared McDonald Date: Sat, 8 Oct 2016 14:10:29 -0400 Subject: [PATCH 04/12] add basic test for config/babel --- config/__tests__/babel.test.js | 19 +++++++++++++++++++ utils/__mocks__/kytConfig.js | 4 +++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/config/__tests__/babel.test.js b/config/__tests__/babel.test.js index 5113c98d7..518fd5200 100644 --- a/config/__tests__/babel.test.js +++ b/config/__tests__/babel.test.js @@ -16,10 +16,15 @@ jest.setMock('path', { }); jest.mock('../../utils/kytConfig'); +const kytConfig = require('../../utils/kytConfig'); const babel = require('../babel'); describe('babel', () => { + beforeEach(() => { + kytConfig().modifyBabelConfig.mockClear(); + }); + it('sets flags', () => { const babelrc = babel(); expect(babelrc.babelrc).toBe(false); @@ -37,4 +42,18 @@ describe('babel', () => { expect(babelrc.env.development.plugins).toEqual([thisResolved]); expect(babelrc.env.development.presets).toEqual([[thisResolved]]); }); + + it('calls kytConfig\'s modifyBabelConfig', () => { + const opts = {}; + babel(opts); + const config = kytConfig(); + expect(config.modifyBabelConfig).toBeCalled(); + expect(config.modifyBabelConfig.mock.calls[0][0]).toEqual(Object.assign({}, stubBabelrc, { + babelrc: false, + cacheDirectory: false, + plugins: [], + presets: [], + })); + expect(config.modifyBabelConfig.mock.calls[0][1]).toBe(opts); + }); }); diff --git a/utils/__mocks__/kytConfig.js b/utils/__mocks__/kytConfig.js index b820d7177..cb4dd8d00 100644 --- a/utils/__mocks__/kytConfig.js +++ b/utils/__mocks__/kytConfig.js @@ -1,4 +1,6 @@ +const modifyBabelConfig = jest.fn(c => c); + module.exports = () => ({ // TODO add more to this mock (this is all we need for now) - modifyBabelConfig: jest.fn(c => c), + modifyBabelConfig, }); From db9dce11f681ebfab67646d855adaaf80e56da1b Mon Sep 17 00:00:00 2001 From: Jared McDonald Date: Sat, 8 Oct 2016 15:17:55 -0400 Subject: [PATCH 05/12] test error path for config/babel --- config/__tests__/babel.test.js | 27 ++++++++++++++++++++++++--- utils/__mocks__/kytConfig.js | 6 ------ 2 files changed, 24 insertions(+), 9 deletions(-) delete mode 100644 utils/__mocks__/kytConfig.js diff --git a/config/__tests__/babel.test.js b/config/__tests__/babel.test.js index 518fd5200..970584851 100644 --- a/config/__tests__/babel.test.js +++ b/config/__tests__/babel.test.js @@ -8,6 +8,7 @@ const stubBabelrc = { }, }, }; +jest.mock('../../cli/logger'); jest.setMock('fs', { readFileSync: () => JSON.stringify(stubBabelrc), }); @@ -15,14 +16,25 @@ jest.setMock('path', { resolve: a => a, }); -jest.mock('../../utils/kytConfig'); -const kytConfig = require('../../utils/kytConfig'); +const modifyBabelConfig = jest.fn(c => c); +const mockKytConfig = () => ({ modifyBabelConfig }); +jest.setMock('../../utils/kytConfig', mockKytConfig); + +const logger = require('../../cli/logger'); const babel = require('../babel'); describe('babel', () => { beforeEach(() => { - kytConfig().modifyBabelConfig.mockClear(); + modifyBabelConfig.mockClear(); + global.process.exit = jest.fn(); + Object.keys(logger).forEach(k => logger[k].mockClear()); + }); + + it('does not call process.exit or logger.error on a successful run', () => { + babel(); + expect(logger.error.mock.calls.length).toBe(0); + expect(global.process.exit.mock.calls.length).toBe(0); }); it('sets flags', () => { @@ -46,6 +58,7 @@ describe('babel', () => { it('calls kytConfig\'s modifyBabelConfig', () => { const opts = {}; babel(opts); + const kytConfig = require('../../utils/kytConfig'); const config = kytConfig(); expect(config.modifyBabelConfig).toBeCalled(); expect(config.modifyBabelConfig.mock.calls[0][0]).toEqual(Object.assign({}, stubBabelrc, { @@ -56,4 +69,12 @@ describe('babel', () => { })); expect(config.modifyBabelConfig.mock.calls[0][1]).toBe(opts); }); + + it('logs an error and exits the process if modifyBabelConfig throws', () => { + const err = new Error('oh no'); + modifyBabelConfig.mockImplementationOnce(() => { throw err; }); + babel(); + expect(logger.error).toBeCalledWith('Error in your kyt.config.js modifyBabelConfig():', err); + expect(global.process.exit).toBeCalledWith(1); + }); }); diff --git a/utils/__mocks__/kytConfig.js b/utils/__mocks__/kytConfig.js deleted file mode 100644 index cb4dd8d00..000000000 --- a/utils/__mocks__/kytConfig.js +++ /dev/null @@ -1,6 +0,0 @@ -const modifyBabelConfig = jest.fn(c => c); - -module.exports = () => ({ - // TODO add more to this mock (this is all we need for now) - modifyBabelConfig, -}); From 835f0a38f942b16bacd5a4f0dd0e11203a359325 Mon Sep 17 00:00:00 2001 From: Jared McDonald Date: Sat, 8 Oct 2016 15:19:37 -0400 Subject: [PATCH 06/12] improve expectation readability --- config/__tests__/babel.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/__tests__/babel.test.js b/config/__tests__/babel.test.js index 970584851..e975f3c8e 100644 --- a/config/__tests__/babel.test.js +++ b/config/__tests__/babel.test.js @@ -33,8 +33,8 @@ describe('babel', () => { it('does not call process.exit or logger.error on a successful run', () => { babel(); - expect(logger.error.mock.calls.length).toBe(0); - expect(global.process.exit.mock.calls.length).toBe(0); + expect(logger.error).not.toBeCalled(); + expect(global.process.exit).not.toBeCalled(); }); it('sets flags', () => { From 3b57e118cd43f857323dd15ccec8bc00c58b034c Mon Sep 17 00:00:00 2001 From: Jared McDonald Date: Sat, 8 Oct 2016 15:30:19 -0400 Subject: [PATCH 07/12] fix double call of modifyBabelConfig in test preprocessor --- utils/jest/preprocessor.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/utils/jest/preprocessor.js b/utils/jest/preprocessor.js index 700e09f99..8b9d1a621 100644 --- a/utils/jest/preprocessor.js +++ b/utils/jest/preprocessor.js @@ -1,12 +1,10 @@ -const babel = require('../../config/babel')(); const babelJest = require('babel-jest'); -const config = require('../kytConfig')(); const options = { type: 'test', environment: 'test', }; -const babelConfigForJest = config.modifyBabelConfig(babel, options); +const babelConfigForJest = require('../../config/babel')(options); module.exports = babelJest.createTransformer(babelConfigForJest); From 5e3dad7debbdf566f95ba80762d0ec4006e8024c Mon Sep 17 00:00:00 2001 From: Jared McDonald Date: Sat, 8 Oct 2016 15:41:23 -0400 Subject: [PATCH 08/12] adds test for utils/jest/preprocessor --- utils/jest/__tests__/preprocessor.test.js | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 utils/jest/__tests__/preprocessor.test.js diff --git a/utils/jest/__tests__/preprocessor.test.js b/utils/jest/__tests__/preprocessor.test.js new file mode 100644 index 000000000..d3636b065 --- /dev/null +++ b/utils/jest/__tests__/preprocessor.test.js @@ -0,0 +1,23 @@ +const fakeTransformer = {}; + +jest.setMock('babel-jest', { + createTransformer: jest.fn(() => fakeTransformer), +}); +const babelJest = require('babel-jest'); + +const fakeBabelResult = {}; + +jest.setMock('../../../config/babel', jest.fn(() => fakeBabelResult)); +const babel = require('../../../config/babel'); + +describe('preprocessor', () => { + it('calls the babel utility and babel-jest\'s createTransformer', () => { + const preprocessor = require('../preprocessor'); // eslint-disable-line global-require + expect(preprocessor).toBe(fakeTransformer); + expect(babel.mock.calls[0][0]).toEqual({ + type: 'test', + environment: 'test', + }); + expect(babelJest.createTransformer).toBeCalledWith(fakeBabelResult); + }); +}); From 5c516591ad13a89c416971550bb30e1f96502335 Mon Sep 17 00:00:00 2001 From: Jared McDonald Date: Sat, 8 Oct 2016 15:43:12 -0400 Subject: [PATCH 09/12] add assertion for the addition of default identity function in kytConfig base --- utils/__tests__/kytConfig.test.js | 1 + 1 file changed, 1 insertion(+) diff --git a/utils/__tests__/kytConfig.test.js b/utils/__tests__/kytConfig.test.js index e9da562cc..a8c9b85b1 100644 --- a/utils/__tests__/kytConfig.test.js +++ b/utils/__tests__/kytConfig.test.js @@ -30,6 +30,7 @@ describe('kytConfig', () => { expect(logger.info).toBeCalled(); expect(typeof config.modifyWebpackConfig).toBe('function'); expect(typeof config.modifyJestConfig).toBe('function'); + expect(typeof config.modifyBabelConfig).toBe('function'); expect(() => { config.productionPublicPath = 'frozen!'; }).toThrow(); From bb9c7be523ba95d73e26804507c4fbd33e1f4ac2 Mon Sep 17 00:00:00 2001 From: Jared McDonald Date: Sat, 8 Oct 2016 15:45:22 -0400 Subject: [PATCH 10/12] don't modify this comment anymore --- utils/buildConfigs.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/utils/buildConfigs.js b/utils/buildConfigs.js index fdf958b97..49d7d918a 100644 --- a/utils/buildConfigs.js +++ b/utils/buildConfigs.js @@ -47,7 +47,7 @@ module.exports = (config, environment = 'development') => { clientConfig = merge.smart(baseConfig(clientOptions), clientConfig(clientOptions)); serverConfig = merge.smart(baseConfig(serverOptions), serverConfig(serverOptions)); - // Modify via userland webpack config + // Modify via userland config try { clientConfig = config.modifyWebpackConfig(clone(clientConfig), clientOptions); serverConfig = config.modifyWebpackConfig(clone(serverConfig), serverOptions); From b789da3fe6021691deebad42d99f3c7d97fc3075 Mon Sep 17 00:00:00 2001 From: Jared McDonald Date: Sat, 8 Oct 2016 16:02:55 -0400 Subject: [PATCH 11/12] add debug option to log out merged babelrc --- config/__tests__/babel.test.js | 16 +++++++++++++++- config/babel.js | 10 ++++++++-- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/config/__tests__/babel.test.js b/config/__tests__/babel.test.js index e975f3c8e..56cd7c981 100644 --- a/config/__tests__/babel.test.js +++ b/config/__tests__/babel.test.js @@ -17,7 +17,7 @@ jest.setMock('path', { }); const modifyBabelConfig = jest.fn(c => c); -const mockKytConfig = () => ({ modifyBabelConfig }); +const mockKytConfig = jest.fn(() => ({ modifyBabelConfig })); jest.setMock('../../utils/kytConfig', mockKytConfig); const logger = require('../../cli/logger'); @@ -27,6 +27,7 @@ const babel = require('../babel'); describe('babel', () => { beforeEach(() => { modifyBabelConfig.mockClear(); + mockKytConfig.mockClear(); global.process.exit = jest.fn(); Object.keys(logger).forEach(k => logger[k].mockClear()); }); @@ -77,4 +78,17 @@ describe('babel', () => { expect(logger.error).toBeCalledWith('Error in your kyt.config.js modifyBabelConfig():', err); expect(global.process.exit).toBeCalledWith(1); }); + + it('does not log out merged config if config.debug is falsy', () => { + babel(); + expect(logger.debug).not.toBeCalled(); + }); + + it('logs out the merged config if config.debug is truthy', () => { + mockKytConfig.mockImplementationOnce(() => ({ debug: true, modifyBabelConfig })); + babel(); + expect(logger.debug).toBeCalled(); + expect(typeof logger.debug.mock.calls[0][0]).toBe('string'); + expect(typeof logger.debug.mock.calls[0][1]).toBe('object'); + }); }); diff --git a/config/babel.js b/config/babel.js index 6ebb8adc1..1f106f031 100644 --- a/config/babel.js +++ b/config/babel.js @@ -1,6 +1,6 @@ const path = require('path'); const fs = require('fs'); -const config = require('../utils/kytConfig')(); +const getKytConfig = require('../utils/kytConfig'); const logger = require('../cli/logger'); // Uses require.resolve to add the full paths to all of the plugins @@ -18,6 +18,8 @@ const resolvePluginsPresets = (babelGroup) => { }; module.exports = (options) => { + const config = getKytConfig(); + // Create the babelrc query for the babel loader. const babelrc = JSON.parse(fs.readFileSync(path.resolve(__dirname, '../.babelrc'), 'utf8')); babelrc.babelrc = false; @@ -30,7 +32,11 @@ module.exports = (options) => { // modify via userland kytConfig's `modifyBabelConfig` try { - return config.modifyBabelConfig(babelrc, options); + const mergedBabelrc = config.modifyBabelConfig(babelrc, options); + if (config.debug) { + logger.debug('Merged babel configuration:', mergedBabelrc); + } + return mergedBabelrc; } catch (error) { logger.error('Error in your kyt.config.js modifyBabelConfig():', error); // return to satisfy eslint `consistent-return` rule From 52965aa6742192659ca0111dd7f868685619aa3f Mon Sep 17 00:00:00 2001 From: Jared McDonald Date: Sat, 8 Oct 2016 20:38:36 -0400 Subject: [PATCH 12/12] merged -> modified --- config/babel.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/config/babel.js b/config/babel.js index 1f106f031..100c6be2c 100644 --- a/config/babel.js +++ b/config/babel.js @@ -30,13 +30,13 @@ module.exports = (options) => { } Object.keys(babelrc.env || {}).forEach(env => resolvePluginsPresets(babelrc.env[env])); - // modify via userland kytConfig's `modifyBabelConfig` try { - const mergedBabelrc = config.modifyBabelConfig(babelrc, options); + // modify via userland kytConfig's `modifyBabelConfig` + const modifiedBabelrc = config.modifyBabelConfig(babelrc, options); if (config.debug) { - logger.debug('Merged babel configuration:', mergedBabelrc); + logger.debug('Modified babel configuration:', modifiedBabelrc); } - return mergedBabelrc; + return modifiedBabelrc; } catch (error) { logger.error('Error in your kyt.config.js modifyBabelConfig():', error); // return to satisfy eslint `consistent-return` rule