Skip to content

Pre-process all ES5 builds #2840

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

Merged
merged 9 commits into from
Apr 1, 2020
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
1 change: 1 addition & 0 deletions packages/firestore/memory/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
"name": "@firebase/firestore/memory",
"description": "A memory-only build of the Cloud Firestore JS SDK.",
"main": "../dist/index.memory.node.cjs.js",
"mainES2017": "../dist/index.memory.node.esm2017.js",
Copy link
Member

Choose a reason for hiding this comment

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

main-esm2017?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

"browser": "../dist/index.memory.cjs.js",
"module": "../dist/index.memory.esm.js",
"esm2017": "../dist/index.memory.esm2017.js",
Expand Down
3 changes: 2 additions & 1 deletion packages/firestore/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"author": "Firebase <[email protected]> (https://firebase.google.com/)",
"scripts": {
"prebuild": "tsc -d --downlevelIteration --declarationDir dist/lib --emitDeclarationOnly && tsc -m es2015 --moduleResolution node scripts/*.ts ",
"build": "rollup -c",
"build": "rollup -c rollup.config.es2017.js && rollup -c rollup.config.es.js",
"build:deps": "lerna run --scope @firebase/'{app,firestore}' --include-dependencies build",
"build:console": "node tools/console.build.js",
"predev": "yarn prebuild",
Expand All @@ -26,6 +26,7 @@
"prepare": "yarn build"
},
"main": "dist/index.node.cjs.js",
"mainES2017": "dist/index.node.esm2017.js",
Copy link
Member

Choose a reason for hiding this comment

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

main-esm2017 to be consistent with the existing convention? We have a lite-esm2017 target in @firebase/app which is used to produce firebase-performance-standalone.es2017.js

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 was looking for a precedent. I am glad there is one :)

"browser": "dist/index.cjs.js",
"module": "dist/index.esm.js",
"esm2017": "dist/index.esm2017.js",
Expand Down
128 changes: 128 additions & 0 deletions packages/firestore/rollup.config.es.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
/**
* @license
* Copyright 2018 Google Inc.
*
* 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.
*/

import * as path from 'path';

import typescriptPlugin from 'rollup-plugin-typescript2';
import sourcemaps from 'rollup-plugin-sourcemaps';
import typescript from 'typescript';
import { terser } from 'rollup-plugin-terser';
import { resolveNodeExterns, resolveBrowserExterns } from './rollup.shared';

import pkg from './package.json';
import memoryPkg from './memory/package.json';

// This file defines the second rollup pipeline and transpiles the ES2017 SDK
// into ES3 code. By splitting the build process into two independent build
Copy link
Member

Choose a reason for hiding this comment

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

I think it should be ES5 code.

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 forgot to look in base configuration and assumed we used the default, which is ES3. Fixed.

// pipelines, we take advantage of tree shaking in ES2017 builds even for
// language levels that don't support tree shaking.

const browserPlugins = [
typescriptPlugin({
typescript,
compilerOptions: {
allowJs: true,
importHelpers: true
Copy link
Member

Choose a reason for hiding this comment

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

It can be removed because It's already set in the root tsconfig.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This didn't work for me when it was not included yesterday, but I will try again. Fingers crossed :)

Update: No size impact so I removed it.

},
include: ['dist/*.js']
}),
terser({
output: {
comments: 'all',
beautify: true
},
mangle: true
}),
sourcemaps()
];

const nodePlugins = [
typescriptPlugin({
typescript,
compilerOptions: {
allowJs: true,
importHelpers: true
},
include: ['dist/*.js']
}),
terser({
Copy link
Member

Choose a reason for hiding this comment

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

Do we need terser again here since we already do it with the same options (comments: all, beautify: true) when producing esm2017 builds?

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 believe the size went up without Terser here, but I will do another round to verify.

Update: The size without Terser is about 4% larger (which I don't quite understand either, since we Terser again before computing the size). I added it back.

Copy link
Member

@Feiyang1 Feiyang1 Apr 1, 2020

Choose a reason for hiding this comment

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

IIUC, we are not doing property mangling here, and we don't care about the size saving by mangling variable names. Developers will do them when building their apps.
Removing terser here can save some build time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, removed.

output: {
comments: 'all',
beautify: true
},
mangle: false
}),
sourcemaps()
];

const browserBuilds = [
{
input: pkg.esm2017,
output: { file: pkg.module, format: 'es', sourcemap: true },
plugins: browserPlugins,
external: resolveBrowserExterns
},
{
input: path.resolve('./memory', memoryPkg.esm2017),
output: {
file: path.resolve('./memory', memoryPkg.module),
format: 'es',
sourcemap: true
},
plugins: browserPlugins,
external: resolveBrowserExterns
},
{
input: pkg.esm2017,
output: { file: pkg.browser, format: 'cjs', sourcemap: true },
plugins: browserPlugins,
external: resolveBrowserExterns
},
{
input: path.resolve('./memory', memoryPkg.esm2017),
output: {
file: path.resolve('./memory', memoryPkg.browser),
format: 'cjs',
sourcemap: true
},
plugins: browserPlugins,
external: resolveBrowserExterns
}
];

const nodeBuilds = [
{
input: pkg.mainES2017,
output: [{ file: pkg.main, format: 'cjs', sourcemap: true }],
plugins: nodePlugins,
external: resolveNodeExterns
},
{
input: path.resolve('./memory', memoryPkg.mainES2017),
output: [
{
file: path.resolve('./memory', memoryPkg.main),
format: 'cjs',
sourcemap: true
}
],
plugins: nodePlugins,
external: resolveNodeExterns
}
];

export default [...browserBuilds, ...nodeBuilds];
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import json from 'rollup-plugin-json';
import typescriptPlugin from 'rollup-plugin-typescript2';
import replace from 'rollup-plugin-replace';
import copy from 'rollup-plugin-copy-assets';
import sourcemaps from 'rollup-plugin-sourcemaps';
import typescript from 'typescript';
import { terser } from 'rollup-plugin-terser';

Expand All @@ -30,10 +29,14 @@ import memoryPkg from './memory/package.json';

import {
appendPrivatePrefixTransformers,
manglePrivatePropertiesOptions
} from './terser.config';

// This Firestore Rollup configuration provides a number of different builds:
manglePrivatePropertiesOptions,
resolveNodeExterns,
resolveBrowserExterns,
browserDeps,
nodeDeps
} from './rollup.shared';

// Firestore is released in a number of different build configurations:
// - Browser builds that support persistence in ES5 CJS and ES5 ESM formats and
// ES2017 in ESM format.
// - In-memory Browser builds that support persistence in ES5 CJS and ES5 ESM
Expand All @@ -45,6 +48,10 @@ import {
// The in-memory builds are roughly 130 KB smaller, but throw an exception
// for calls to `enablePersistence()` or `clearPersistence()`.
//
// We use two different rollup pipelines to take advantage of tree shaking in
Copy link
Member

Choose a reason for hiding this comment

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

Probably mention that it's because rollup is not able to treeshake Typescript classes compiled to ES5.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

// the ES2017 builds. The build pipeline in this file produces ES2017 builds
// that are consumed by `rollup.config.es.js`.
//
// All browser builds rely on Terser's property name mangling to reduce code
// size.
//
Expand All @@ -53,22 +60,6 @@ import {

// MARK: Browser builds

const browserDeps = Object.keys(
Object.assign({}, pkg.peerDependencies, pkg.dependencies)
);

const nodeDeps = [...browserDeps, 'util', 'path'];

/** Resolves the external dependencies for the browser build. */
function resolveBrowserExterns(id) {
return browserDeps.some(dep => id === dep || id.startsWith(`${dep}/`));
}

/** Resolves the external dependencies for the Node build. */
function resolveNodeExterns(id) {
return nodeDeps.some(dep => id === dep || id.startsWith(`${dep}/`));
}

/**
* Resolves the external dependencies for the Memory-based Firestore
* implementation. Verifies that no persistence sources are used by Firestore's
Expand Down Expand Up @@ -99,95 +90,44 @@ export function resolveMemoryExterns(deps, externsId, referencedBy) {
return deps.some(dep => externsId === dep || externsId.startsWith(`${dep}/`));
}

const es5BuildPlugins = [
typescriptPlugin({
typescript,
transformers: appendPrivatePrefixTransformers,
cacheRoot: `./.cache/es5.mangled/`
}),
json(),
terser(manglePrivatePropertiesOptions)
];

const es2017BuildPlugins = [
const browserBuildPlugins = [
typescriptPlugin({
typescript,
tsconfigOverride: {
compilerOptions: {
target: 'es2017'
}
},
cacheRoot: './.cache/es2017.mangled/',
clean: true,
transformers: appendPrivatePrefixTransformers
}),
json({ preferConst: true }),
terser(manglePrivatePropertiesOptions)
];

const browserBuilds = [
// ES5 ESM Build (with persistence)
{
input: 'index.ts',
output: { file: pkg.module, format: 'es', sourcemap: true },
plugins: es5BuildPlugins,
external: resolveBrowserExterns
},
// ES5 ESM Build (memory-only)
{
input: 'index.memory.ts',
output: {
file: path.resolve('./memory', memoryPkg.module),
format: 'es',
sourcemap: true
},
plugins: es5BuildPlugins,
external: (id, referencedBy) =>
resolveMemoryExterns(browserDeps, id, referencedBy)
},
// ES2017 ESM build (with persistence)
// Persistence build
{
input: 'index.ts',
output: {
file: pkg.esm2017,
format: 'es',
sourcemap: true
},
plugins: es2017BuildPlugins,
plugins: browserBuildPlugins,
external: resolveBrowserExterns
},
// ES2017 ESM build (memory-only)
// Memory-only build
{
input: 'index.memory.ts',
output: {
file: path.resolve('./memory', memoryPkg.esm2017),
format: 'es',
sourcemap: true
},
plugins: es2017BuildPlugins,
plugins: browserBuildPlugins,
external: (id, referencedBy) =>
resolveMemoryExterns(browserDeps, id, referencedBy)
},
// ES5 CJS Build (with persistence)
//
// This build is based on the mangling in the ESM build above, since
// Terser's Property name mangling doesn't work well with CJS's syntax.
{
input: pkg.module,
output: { file: pkg.browser, format: 'cjs', sourcemap: true },
plugins: [sourcemaps()]
},
// ES5 CJS Build (memory-only)
//
// This build is based on the mangling in the ESM build above, since
// Terser's Property name mangling doesn't work well with CJS's syntax.
{
input: path.resolve('./memory', memoryPkg.module),
output: {
file: path.resolve('./memory', memoryPkg.browser),
format: 'cjs',
sourcemap: true
},
plugins: [sourcemaps()]
}
];

Expand All @@ -196,7 +136,12 @@ const browserBuilds = [
const nodeBuildPlugins = [
typescriptPlugin({
typescript,
cacheRoot: `./.cache/node/`
tsconfigOverride: {
compilerOptions: {
target: 'es2017'
}
},
clean: true
}),
json(),
// Needed as we also use the *.proto files
Expand All @@ -209,20 +154,20 @@ const nodeBuildPlugins = [
];

const nodeBuilds = [
// ES5 CJS build (with persistence)
// Persistence build
{
input: 'index.node.ts',
output: [{ file: pkg.main, format: 'cjs', sourcemap: true }],
output: [{ file: pkg.mainES2017, format: 'es', sourcemap: true }],
plugins: nodeBuildPlugins,
external: resolveNodeExterns
},
// ES5 CJS build (memory-only)
// Memory-only build
{
input: 'index.node.memory.ts',
output: [
{
file: path.resolve('./memory', memoryPkg.main),
format: 'cjs',
file: path.resolve('./memory', memoryPkg.mainES2017),
format: 'es',
sourcemap: true
}
],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,24 @@ import { externs } from './externs.json';
import { renameInternals } from './scripts/rename-internals';
import { extractPublicIdentifiers } from './scripts/extract-api';

import pkg from './package.json';

export const browserDeps = Object.keys(
Object.assign({}, pkg.peerDependencies, pkg.dependencies)
);

export const nodeDeps = [...browserDeps, 'util', 'path'];

/** Resolves the external dependencies for the browser build. */
export function resolveBrowserExterns(id) {
return browserDeps.some(dep => id === dep || id.startsWith(`${dep}/`));
}

/** Resolves the external dependencies for the Node build. */
export function resolveNodeExterns(id) {
return nodeDeps.some(dep => id === dep || id.startsWith(`${dep}/`));
}

const externsPaths = externs.map(p => path.resolve(__dirname, '../../', p));
const publicIdentifiers = extractPublicIdentifiers(externsPaths);

Expand Down
2 changes: 1 addition & 1 deletion packages/firestore/test/util/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,7 @@ export function byteStringFromString(value: string): ByteString {
return ByteString.fromBase64String(base64);
}

/**
/**
* Decodes a base 64 decoded string.
*
* Note that this is typed to accept Uint8Arrays to match the types used
Expand Down