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.

Sign up for GitHub

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

Jump to bottom

refactor(): remove in-browser compilation support #4317

Merged
merged 1 commit into from
Jun 7, 2023
Merged

refactor(): remove in-browser compilation support #4317

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion jest.config.js
View file
Open in desktop
alicewriteswrongs marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@ module.exports = {
moduleNameMapper: {
'@app-data': '<rootDir>/internal/app-data/index.cjs',
'@app-globals': '<rootDir>/internal/app-globals/index.cjs',
'@compiler-deps': '<rootDir>/src/compiler/sys/modules/compiler-deps.ts',
'@platform': '<rootDir>/internal/testing/index.js',
'@runtime': '<rootDir>/internal/testing/index.js',
'@stencil/core/cli': '<rootDir>/cli/index.js',
'@stencil/core/compiler': '<rootDir>/compiler/stencil.js',
'@stencil/core/mock-doc': '<rootDir>/mock-doc/index.cjs',
'@stencil/core/testing': '<rootDir>/testing/index.js',
'@sys-api-node': '<rootDir>/sys/node/index.js',
alicewriteswrongs marked this conversation as resolved.
Show resolved Hide resolved
'@utils': '<rootDir>/src/utils',
'^typescript$': '<rootDir>/scripts/build/typescript-modified-for-jest.js',
},
Expand Down
35 changes: 35 additions & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@
"@types/inquirer": "^7.3.1",
"@types/jest": "^27.0.3",
"@types/listr": "^0.14.4",
"@types/mock-fs": "^4.13.1",
"@types/node": "^20.1.1",
"@types/pixelmatch": "^5.2.4",
"@types/pngjs": "^6.0.1",
Expand Down Expand Up @@ -106,6 +107,7 @@
"merge-source-map": "^1.1.0",
"mime-db": "^1.46.0",
"minimatch": "5.1.6",
"mock-fs": "^5.2.0",
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 added a new library for mocking out the node fs module during testing. I'll highlight how it's used in some test files below, but basically it stands up an in-memory filesystem and patches the fs module to make writes and reads against that instead of the disk. This lets us simplify some code where under certain circumstances things would use an in-memory stencil CompilerySystem and in others use the NodeSys. Since we're removing support for running in the browser we can default to the node system in more places without worry, and this package makes it fairly straightforward to test that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Open to other thoughts about how to do this if we don't want to add a dependency though!

Copy link
Member

Choose a reason for hiding this comment

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

I don't have an issue with dependencies - especially if it means we don't have to reinvent and maintain the behavior ourselves - but I know there is more hesitation with them when it comes to Stencil. As a dev-dep though, I think this would be fine

"node-fetch": "3.3.1",
"open": "^9.0.0",
"open-in-editor": "2.2.0",
Expand Down
7 changes: 2 additions & 5 deletions scripts/bundles/compiler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,13 @@ import type { OutputChunk, RollupOptions, RollupWarning, TransformResult } from
import sourcemaps from 'rollup-plugin-sourcemaps';

import { getBanner } from '../utils/banner';
import { NODE_BUILTINS } from '../utils/constants';
import type { BuildOptions } from '../utils/options';
import { writePkgJson } from '../utils/write-pkg-json';
import { aliasPlugin } from './plugins/alias-plugin';
import { inlinedCompilerDepsPlugin } from './plugins/inlined-compiler-deps-plugin';
import { parse5Plugin } from './plugins/parse5-plugin';
import { replacePlugin } from './plugins/replace-plugin';
import { sizzlePlugin } from './plugins/sizzle-plugin';
import { sysModulesPlugin } from './plugins/sys-modules-plugin';
import { terserPlugin } from './plugins/terser-plugin';
import { typescriptSourcePlugin } from './plugins/typescript-source-plugin';

Expand Down Expand Up @@ -67,6 +66,7 @@ export async function compiler(opts: BuildOptions) {
const rollupWatchPath = join(opts.nodeModulesDir, 'rollup', 'dist', 'es', 'shared', 'watch.js');
const compilerBundle: RollupOptions = {
input: join(inputDir, 'index.js'),
external: NODE_BUILTINS,
output: {
format: 'cjs',
file: join(opts.output.compilerDir, compilerFileName),
Expand Down Expand Up @@ -154,14 +154,11 @@ export async function compiler(opts: BuildOptions) {
};
},
},
inlinedCompilerDepsPlugin(opts, inputDir),
parse5Plugin(opts),
sizzlePlugin(opts),
aliasPlugin(opts),
sysModulesPlugin(inputDir),
rollupNodeResolve({
mainFields: ['module', 'main'],
preferBuiltins: false,
}),
rollupCommonjs({
transformMixedEsModules: false,
Expand Down
4 changes: 3 additions & 1 deletion scripts/bundles/plugins/alias-plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ export function aliasPlugin(opts: BuildOptions): Plugin {
['@hydrate-factory', '@stencil/core/hydrate-factory'],
['@stencil/core/mock-doc', '@stencil/core/mock-doc'],
['@stencil/core/testing', '@stencil/core/testing'],
['@sys-api-node', './index.js'],
['@dev-server-process', './server-process.js'],
]);

Expand Down Expand Up @@ -54,6 +53,9 @@ export function aliasPlugin(opts: BuildOptions): Plugin {
if (id === '@environment') {
return join(opts.buildDir, 'compiler', 'sys', 'environment.js');
}
if (id === '@sys-api-node') {
return join(opts.buildDir, 'sys', 'node', 'index.js');
}
if (helperResolvers.has(id)) {
return join(opts.bundleHelpersDir, `${id}.js`);
}
Expand Down
77 changes: 0 additions & 77 deletions scripts/bundles/plugins/inlined-compiler-deps-plugin.ts
View file
Open in desktop

This file was deleted.

31 changes: 0 additions & 31 deletions scripts/bundles/plugins/sys-modules-plugin.ts
View file
Open in desktop

This file was deleted.

6 changes: 4 additions & 2 deletions scripts/bundles/sys-node.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import type { RollupOptions } from 'rollup';
import webpack, { Configuration } from 'webpack';

import { getBanner } from '../utils/banner';
import { NODE_BUILTINS } from '../utils/constants';
import type { BuildOptions } from '../utils/options';
import { writePkgJson } from '../utils/write-pkg-json';
import { aliasPlugin } from './plugins/alias-plugin';
Expand Down Expand Up @@ -38,7 +39,7 @@ export async function sysNode(opts: BuildOptions) {
preferConst: true,
freeze: false,
},
external: ['child_process', 'crypto', 'events', 'https', 'path', 'readline', 'os', 'util'],
external: NODE_BUILTINS,
plugins: [
relativePathPlugin('glob', './glob.js'),
relativePathPlugin('graceful-fs', './graceful-fs.js'),
Expand Down Expand Up @@ -69,7 +70,7 @@ export async function sysNode(opts: BuildOptions) {
preferConst: true,
freeze: false,
},
external: ['child_process', 'crypto', 'events', 'https', 'path', 'readline', 'os', 'util'],
external: NODE_BUILTINS,
plugins: [
{
name: 'sysNodeWorkerAlias',
Expand All @@ -82,6 +83,7 @@ export async function sysNode(opts: BuildOptions) {
}
},
},
relativePathPlugin('@sys-api-node', './index.js'),
rollupResolve({
preferBuiltins: true,
}),
Expand Down
6 changes: 5 additions & 1 deletion scripts/test/validate-build.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { dirname, join, relative } from 'path';
import { rollup } from 'rollup';
import ts, { ModuleResolutionKind, ScriptTarget } from 'typescript';

import { NODE_BUILTINS } from '../utils/constants';
import { BuildOptions, getOptions } from '../utils/options';
import { PackageData } from '../utils/write-pkg-json';

Expand Down Expand Up @@ -335,8 +336,11 @@ async function validateModuleTreeshake(opts: BuildOptions, moduleName: string, e
const outputFile = join(opts.scriptsBuildDir, `treeshake_${moduleName}.js`);

const bundle = await rollup({
external: NODE_BUILTINS,
input: virtualInputId,
treeshake: true,
treeshake: {
moduleSideEffects: false,
},
Comment on lines +341 to +343
Copy link
Contributor Author

Choose a reason for hiding this comment

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

without this a bare import "path" sticks around

Copy link
Member

Choose a reason for hiding this comment

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

Interesting 🤔 Any idea which usage of 'path' it's from? Is it just in the CLI module?

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 unless you explicitly set moduleSideEffects: false rollup will assume that importing a module could have an important side effect and so will never tree shake it away —– given the dynamic nature of JS there's not really an easy way to statically analyze a module to tell whether it will or won't have a side effect, so the safe thing is to default to not removing it.

I think it's this usage:

stencil/src/cli/task-generate.ts

Line 2 in f9cb2a6

import { join, parse, relative } from 'path';

previously the 'generate' task used the path export from src/compiler/index.ts instead

plugins: [
{
name: 'stencilResolver',
Expand Down
8 changes: 1 addition & 7 deletions src/compiler/sys/modules/module.ts → scripts/utils/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/**
* Node builtin modules as of v14.5.0
* Node built-ins that we mark as external when building Stencil
*/
export const NODE_BUILTINS = [
'_http_agent',
Expand Down Expand Up @@ -58,9 +58,3 @@ export const NODE_BUILTINS = [
'worker_threads',
'zlib',
];

export default class Module {
static get builtinModules() {
return NODE_BUILTINS;
}
}
3 changes: 2 additions & 1 deletion scripts/utils/test/options.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { path } from '../../../compiler';
import path from 'path';

import { BuildOptions, getOptions } from '../options';
import * as Vermoji from '../vermoji';

Expand Down
12 changes: 6 additions & 6 deletions src/cli/run.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { hasError, isFunction, shouldIgnoreError } from '@utils';

import { createLogger } from '../compiler/sys/logger/console-logger';
import type * as d from '../declarations';
import { ValidatedConfig } from '../declarations';
import { createConfigFlags } from './config-flags';
Expand Down Expand Up @@ -114,13 +113,14 @@ export const runTask = async (
coreCompiler: CoreCompiler,
config: d.Config,
task: d.TaskCommand,
sys?: d.CompilerSystem
sys: d.CompilerSystem
): Promise<void> => {
const logger = config.logger ?? createLogger();
const flags = createConfigFlags(config.flags ?? { task });
config.logger = logger;
config.flags = flags;
config.sys = sys ?? config.sys ?? coreCompiler.createSystem({ logger });

if (!config.sys) {
config.sys = sys;
}
const strictConfig: ValidatedConfig = coreCompiler.validateConfig(config, {}).config;

switch (task) {
Expand All @@ -134,7 +134,7 @@ export const runTask = async (

case 'generate':
case 'g':
await taskGenerate(coreCompiler, strictConfig);
await taskGenerate(strictConfig);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the coreCompiler was only being passed down here to get coreCompiler.path, but since we can now just directly depend on the node.js path module we can get rid of that 🎉

break;

case 'help':
Expand Down
Loading