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

Chore: Backport recent refactorings #1648

Merged
merged 7 commits into from
Feb 2, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/silver-taxis-sparkle.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'preact-cli': patch
---

Ensures the public path is normalized when registering service workers
1 change: 1 addition & 0 deletions .eslintignore
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
**/node_modules
**/tests/output
**/*.d.ts
10 changes: 1 addition & 9 deletions .eslintrc
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,5 @@
"react/jsx-no-undef": 2,
"react/jsx-uses-react": 2,
"react/jsx-uses-vars": 2
},
"overrides": [
{
"files": ["**/*.ts"],
"rules": {
"no-undef": "off"
}
}
]
}
}
15 changes: 15 additions & 0 deletions jsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
{
"compilerOptions": {
"target": "ESNext",
"moduleResolution": "Node",
"allowJs": true,
"checkJs": true,
"noEmit": true,
"resolveJsonModule": true,
"allowSyntheticDefaultImports": true,
"jsx": "react",
"jsxFactory": "h",
"jsxFragmentFactory": "Fragment"
},
"include": ["packages/**/*"]
}
11 changes: 11 additions & 0 deletions packages/cli/global.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
declare global {
const __webpack_public_path__: string;
namespace jest {
interface Matchers<R> {
toBeCloseInSize(receivedSize: number, expectedSize: number): R;
toFindMatchingKey(receivedKey: string): R;
}
}
}

export {};
4 changes: 2 additions & 2 deletions packages/cli/lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ const envinfo = require('envinfo');
const sade = require('sade');
const notifier = require('update-notifier');
const { error } = require('./util');
const pkg = require('../package');
const pkg = require('../package.json');

const ver = process.version;
const min = pkg.engines.node;
Expand All @@ -12,7 +12,7 @@ if (
.substring(1)
.localeCompare(min.match(/\d+/g).join('.'), 'en', { numeric: true }) === -1
) {
return error(
error(
`You are using Node ${ver} but preact-cli requires Node ${min}. Please upgrade Node to continue!`,
1
);
Expand Down
5 changes: 0 additions & 5 deletions packages/cli/lib/lib/constants.js

This file was deleted.

15 changes: 8 additions & 7 deletions packages/cli/lib/lib/entry.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,19 @@ if (process.env.NODE_ENV === 'development') {

// only add a debug sw if webpack service worker is not requested.
if (process.env.ADD_SW === undefined && 'serviceWorker' in navigator) {
// eslint-disable-next-line no-undef
navigator.serviceWorker.register(__webpack_public_path__ + 'sw-debug.js');
navigator.serviceWorker.register(
normalizeURL(__webpack_public_path__) + 'sw-debug.js'
);
} else if (process.env.ADD_SW && 'serviceWorker' in navigator) {
// eslint-disable-next-line no-undef
navigator.serviceWorker.register(
__webpack_public_path__ + (process.env.ES_BUILD ? 'sw-esm.js' : 'sw.js')
normalizeURL(__webpack_public_path__) +
(process.env.ES_BUILD ? 'sw-esm.js' : 'sw.js')
);
}
} else if (process.env.ADD_SW && 'serviceWorker' in navigator) {
// eslint-disable-next-line no-undef
navigator.serviceWorker.register(
__webpack_public_path__ + (process.env.ES_BUILD ? 'sw-esm.js' : 'sw.js')
normalizeURL(__webpack_public_path__) +
(process.env.ES_BUILD ? 'sw-esm.js' : 'sw.js')
);
}

Expand Down Expand Up @@ -57,7 +58,7 @@ if (typeof app === 'function') {
hydrate &&
currentURL === normalizeURL(location.pathname);
const doRender = canHydrate ? hydrate : render;
root = doRender(h(app, { CLI_DATA }), document.body, root);
doRender(h(app, { CLI_DATA }), document.body, root);
};

if (module.hot) module.hot.accept('preact-cli-entrypoint', init);
Expand Down
3 changes: 1 addition & 2 deletions packages/cli/lib/lib/webpack/render-html-plugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ const prerender = require('./prerender');
const createLoadManifest = require('./create-load-manifest');
const { warn } = require('../../util');
const { info } = require('../../util');
const { PRERENDER_DATA_FILE_NAME } = require('../constants');

const PREACT_FALLBACK_URL = '/200.html';

Expand Down Expand Up @@ -181,7 +180,7 @@ class PrerenderDataExtractPlugin {
// We dont build prerender data for `200.html`. It can re-use the one for homepage.
return;
}
let path = this.location_ + PRERENDER_DATA_FILE_NAME;
let path = this.location_ + 'preact_prerender_data.json';
if (path.startsWith('/')) {
path = path.substr(1);
}
Expand Down
16 changes: 0 additions & 16 deletions packages/cli/lib/lib/webpack/run-webpack.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,22 +17,6 @@ async function devBuild(env) {

let compiler = webpack(config);
return new Promise((res, rej) => {
compiler.hooks.emit.tapAsync('CliDevPlugin', (compilation, callback) => {
let missingDeps = compilation.missingDependencies;
let nodeModulesPath = resolve(__dirname, '../../../node_modules');

// ...tell webpack to watch node_modules recursively until they appear.
if (
Array.from(missingDeps).some(
file => file.indexOf(nodeModulesPath) !== -1
)
) {
compilation.contextDependencies.push(nodeModulesPath);
}

callback();
});

Comment on lines -20 to -35
Copy link
Member Author

Choose a reason for hiding this comment

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

This was adopted from CRA's WatchMissingNodeModulesPlugin, however, it seems to have been broken for a few years. This path that it watches resolves to <project-name>/node_modules/preact-cli/node_modules which is pretty pointless, as nothing will ever exist/be added to that.

Rather than correct it, I've elected to remove it after seeing it was recently cut from CRA in v5. It apparently was having a rather large performance impact which they either couldn't solve or decided it wasn't worth solving. Not entirely sure which.

I figure there's probably no reason to doubt the performance issues claims, so I cut it.

compiler.hooks.beforeCompile.tap('CliDevPlugin', () => {
if (env['clear']) clear(true);
});
Expand Down
20 changes: 5 additions & 15 deletions packages/cli/lib/lib/webpack/webpack-base-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,21 +16,10 @@ const PnpWebpackPlugin = require(`pnp-webpack-plugin`);

function readJson(file) {
try {
return JSON.parse(readFileSync(file));
return JSON.parse(readFileSync(file, 'utf8'));
} catch (e) {}
}

// attempt to resolve a dependency, giving $CWD/node_modules priority:
// function resolveDep(dep, cwd) {
// try {
// return requireRelative.resolve(dep, cwd || process.cwd());
// } catch (e) {}
// try {
// return require.resolve(dep);
// } catch (e) {}
// return dep;
// }

function findAllNodeModules(startDir) {
let dir = path.resolve(startDir);
let dirs = [];
Expand Down Expand Up @@ -70,6 +59,9 @@ function getSassConfiguration(...includePaths) {
return config;
}

/**
* @returns {import('webpack').Configuration}
*/
module.exports = function createBaseConfig(env) {
const { cwd, isProd, isWatch, src, source } = env;
const babelConfigFile = env.babelConfig || '.babelrc';
Expand Down Expand Up @@ -331,7 +323,7 @@ module.exports = function createBaseConfig(env) {
? '[name].chunk.[contenthash:5].css'
: '[name].chunk.css',
}),
new ProgressBarPlugin({
ProgressBarPlugin({
format:
'\u001b[97m\u001b[44m Build \u001b[49m\u001b[39m [:bar] \u001b[32m\u001b[1m:percent\u001b[22m\u001b[39m (:elapseds) \u001b[2m:msg\u001b[22m',
renderThrottle: 100,
Expand Down Expand Up @@ -388,5 +380,3 @@ module.exports = function createBaseConfig(env) {
},
};
};

module.exports.readJson = readJson;
9 changes: 9 additions & 0 deletions packages/cli/lib/lib/webpack/webpack-client-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ const cleanFilename = name =>
''
);

/**
* @returns {Promise<import('webpack').Configuration>}
*/
async function clientConfig(env) {
const { isProd, source, src, cwd /*, port? */ } = env;
const IS_SOURCE_PREACT_X_OR_ABOVE = isInstalledVersionPreactXOrAbove(cwd);
Expand Down Expand Up @@ -190,6 +193,9 @@ function getBabelEsmPlugin(config) {
return esmPlugins;
}

/**
* @returns {import('webpack').Configuration}
*/
function isProd(config) {
let limit = 200 * 1000; // 200kb
const prodConfig = {
Expand Down Expand Up @@ -274,6 +280,9 @@ function isProd(config) {
return prodConfig;
}

/**
* @returns {import('webpack').Configuration}
*/
function isDev(config) {
const { cwd, src, refresh } = config;

Expand Down
3 changes: 3 additions & 0 deletions packages/cli/lib/lib/webpack/webpack-server-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ const { resolve } = require('path');
const { merge } = require('webpack-merge');
const baseConfig = require('./webpack-base-config');

/**
* @returns {import('webpack').Configuration}
*/
function serverConfig(env) {
return {
entry: {
Expand Down
2 changes: 2 additions & 0 deletions packages/cli/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
"bugs": "https://github.com/preactjs/preact-cli/issues",
"homepage": "https://github.com/preactjs/preact-cli",
"devDependencies": {
"@types/express": "^4.17.13",
"@types/jest": "^27.4.0",
"html-looks-like": "^1.0.2",
"jest": "^27.0.1",
"less": "^4.1.1",
Expand Down
48 changes: 16 additions & 32 deletions packages/cli/tests/build.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,12 @@ const { existsSync } = require('fs');
const { readFile } = require('fs').promises;
const looksLike = require('html-looks-like');
const { create, build } = require('./lib/cli');
const { snapshot, hasKey, isWithin } = require('./lib/utils');
const { snapshot } = require('./lib/utils');
const { subject } = require('./lib/output');
const images = require('./images/build');
const { promisify } = require('util');
const glob = promisify(require('glob').glob);
const minimatch = require('minimatch');

const prerenderUrlFiles = [
'prerender-urls.json',
Expand Down Expand Up @@ -35,13 +36,15 @@ function getRegExpFromMarkup(markup) {
return new RegExp(minifiedMarkup);
}

function testMatch(src, tar) {
let k, tmp;
let keys = Object.keys(tar);
expect(Object.keys(src)).toHaveLength(keys.length);
for (k in src) {
expect(hasKey(k, keys)).toBeTruthy();
if (!isWithin(src[k], tar[tmp])) return false;
function testMatch(received, expected) {
let receivedKeys = Object.keys(received);
let expectedKeys = Object.keys(expected);
expect(receivedKeys).toHaveLength(expectedKeys.length);
for (let key in expected) {
const receivedKey = receivedKeys.find(k => minimatch(k, key));
expect(key).toFindMatchingKey(receivedKey);

expect(receivedKey).toBeCloseInSize(received[receivedKey], expected[key]);
}
}

Expand Down Expand Up @@ -184,15 +187,16 @@ describe('preact build', () => {
expect(existsSync(file)).toBe(true);
});

it('should use template from the code folder', async () => {
// app with custom template set via preact.config.js
it('should use custom `template.html`', async () => {
let dir = await subject('custom-template');
await build(dir);

let file = join(dir, 'build/index.html');
let html = await readFile(file, 'utf-8');

looksLike(html, images.template);
expect(html).toEqual(
expect.stringMatching(getRegExpFromMarkup(images.template))
);
});

it('should patch global location object', async () => {
Expand All @@ -217,28 +221,8 @@ describe('preact build', () => {
expect(existsSync(file)).toBe(true);
});

it('should inject preact.* variables into template', async () => {
let dir = await subject('custom-template-2');
await build(dir);

let file = join(dir, 'build/index.html');
let html = await readFile(file, 'utf-8');

looksLike(html, images.templateReplaced);
});

it('should replace title with <%= preact.title %>', async () => {
let dir = await subject('custom-template-3');
await build(dir);

let file = join(dir, 'build/index.html');
let html = await readFile(file, 'utf-8');

looksLike(html, images.templateReplaced);
});

it('should error out for invalid argument', async () => {
let dir = await subject('custom-template-3');
let dir = await subject('custom-template');
const mockExit = jest.spyOn(process, 'exit').mockImplementation(() => {});
expect(build(dir, { 'service-worker': false })).rejects.toEqual(
new Error('Invalid argument found.')
Expand Down