From a65ef9983badff00fd6e97e7192186493f16f60c Mon Sep 17 00:00:00 2001 From: Joey Parrish Date: Wed, 29 Apr 2020 17:20:50 -0700 Subject: [PATCH] Remove partial exports, broken in new compiler The old compiler would let us export a static method on a class without exporting the whole class and its constructor. The new compiler silently ignores `@export` for any methods of a non-exported class. This means that for the latest Closure Compiler, we must export any class with exported static methods. In some cases, these are non-utility classes with constructors we'd rather not export, but the constructor is implicitly exported by exporting the class itself. This was initially caught by the integration tests. The error wasn't especially helpful, so I added a try/catch to loadShaka that makes the error more apparent: TypeError: Cannot read property 'registerParserByMime' of undefined To make sure we do not make this mistake again, I've added a check to the extern generator, which was already able to detect these types of classes. I don't know a compiler-based way to do it, since the compiler silently ignores the export annotations in these cases. Issue #2528 Change-Id: I797c75a8098b0bb3cf837588569f878253dec2cf --- build/generateExterns.js | 25 +++++++++++++++++++++++++ lib/media/manifest_parser.js | 3 ++- lib/offline/storage_muxer.js | 4 ++++ lib/text/text_engine.js | 8 +++++++- lib/util/dom_utils.js | 1 + lib/util/uint8array_utils.js | 3 ++- test/test/util/util.js | 23 ++++++++++++++++------- 7 files changed, 57 insertions(+), 10 deletions(-) diff --git a/build/generateExterns.js b/build/generateExterns.js index 50e4313e1..f735cf280 100755 --- a/build/generateExterns.js +++ b/build/generateExterns.js @@ -41,6 +41,8 @@ const fs = require('fs'); // The annotations we will consider "exporting" a symbol. const EXPORT_REGEX = /@(?:export|exportInterface|expose)\b/; +// TODO: revisit this when Closure Compiler supports partially-exported classes. +let partiallyExportedClassesDetected = false; /** * Topological sort of general objects using a DFS approach. @@ -736,12 +738,29 @@ function generateExterns(names, inputPath) { } else if (isPartiallyExportedClassAssignmentNode(node)) { // Some classes are not exported, but contain exported members. These // need to have externs generated, too. + + // But wait! The latest compiler won't actually export those correctly! + // TODO: File a bug against the Closure Compiler. + // In the mean time, log these now and throw an error at the end to make + // sure we are generating usable releases. This tends to affect our + // plugin registration APIs, and apps should definitely be able to use + // those! + if (!partiallyExportedClassesDetected) { + partiallyExportedClassesDetected = true; + console.log('The Closure Compiler does not handle partially-exported ' + + 'classes correctly! The following classes need to be exported:'); + } + + const name = getIdentifierString(node.expression.left); + console.log(' * ' + name); + return createExternFromPartiallyExportedClassAssignmentNode(names, node); } else { // Ignore anything else, and don't generate any externs. return ''; } }); + const externs = rawExterns.join(''); return { @@ -779,6 +798,12 @@ function main(args) { const names = new Set(); const results = inputPaths.map((path) => generateExterns(names, path)); + // TODO: revisit this when the compiler supports partially-exported classes. + if (partiallyExportedClassesDetected) { + throw new Error( + 'Partially exported classes are not supported in the compiler!'); + } + // Sort them in dependency order. const sorted = topologicalSort(results, /* getDeps= */ (object) => { return object.requires.map((id) => { diff --git a/lib/media/manifest_parser.js b/lib/media/manifest_parser.js index 1915ef464..3b9d78456 100644 --- a/lib/media/manifest_parser.js +++ b/lib/media/manifest_parser.js @@ -12,9 +12,10 @@ goog.require('shaka.util.Error'); goog.require('shaka.util.Platform'); +// TODO: revisit this when Closure Compiler supports partially-exported classes. /** * @summary An interface to register manifest parsers. - * @exportDoc + * @export */ shaka.media.ManifestParser = class { /** diff --git a/lib/offline/storage_muxer.js b/lib/offline/storage_muxer.js index 20c7823fa..acea0bddf 100644 --- a/lib/offline/storage_muxer.js +++ b/lib/offline/storage_muxer.js @@ -39,6 +39,7 @@ shaka.offline.StorageCellPath; shaka.offline.StorageCellHandle; +// TODO: revisit this when Closure Compiler supports partially-exported classes. /** * StorageMuxer is responsible for managing StorageMechanisms and addressing * cells. The primary purpose of the muxer is to give the caller the correct @@ -54,6 +55,7 @@ shaka.offline.StorageCellHandle; * |findAll|) into a cell, which it then returns. * * @implements {shaka.util.IDestroyable} + * @export */ shaka.offline.StorageMuxer = class { constructor() { @@ -65,11 +67,13 @@ shaka.offline.StorageMuxer = class { this.mechanisms_ = new Map(); } + // TODO: revisit this when the compiler supports partially-exported classes. /** * Free all resources used by the muxer, mechanisms, and cells. This should * not affect the stored content. * * @override + * @export */ destroy() { /** @type {!Array.} */ diff --git a/lib/text/text_engine.js b/lib/text/text_engine.js index 3816248f2..96d894e9c 100644 --- a/lib/text/text_engine.js +++ b/lib/text/text_engine.js @@ -13,9 +13,11 @@ goog.require('shaka.util.IDestroyable'); goog.require('shaka.util.MimeUtils'); +// TODO: revisit this when Closure Compiler supports partially-exported classes. /** * @summary Manages text parsers and cues. * @implements {shaka.util.IDestroyable} + * @export */ shaka.text.TextEngine = class { /** @param {shaka.extern.TextDisplayer} displayer */ @@ -91,7 +93,11 @@ shaka.text.TextEngine = class { return false; } - /** @override */ + // TODO: revisit this when the compiler supports partially-exported classes. + /** + * @override + * @export + */ destroy() { this.parser_ = null; this.displayer_ = null; diff --git a/lib/util/dom_utils.js b/lib/util/dom_utils.js index fa7817ade..13297b7c8 100644 --- a/lib/util/dom_utils.js +++ b/lib/util/dom_utils.js @@ -9,6 +9,7 @@ goog.provide('shaka.util.Dom'); goog.require('goog.asserts'); +// TODO: revisit this when Closure Compiler supports partially-exported classes. /** @export */ shaka.util.Dom = class { /** diff --git a/lib/util/uint8array_utils.js b/lib/util/uint8array_utils.js index eb7039f3f..3d003ae0a 100644 --- a/lib/util/uint8array_utils.js +++ b/lib/util/uint8array_utils.js @@ -11,9 +11,10 @@ goog.require('shaka.util.Iterables'); goog.require('shaka.util.StringUtils'); +// TODO: revisit this when Closure Compiler supports partially-exported classes. /** * @summary A set of Uint8Array utility functions. - * @exportDoc + * @export */ shaka.util.Uint8ArrayUtils = class { /** diff --git a/test/test/util/util.js b/test/test/util/util.js index 3ded25c6a..cfe120be1 100644 --- a/test/test/util/util.js +++ b/test/test/util/util.js @@ -322,14 +322,23 @@ shaka.test.Util = class { // Load the compiled library as a module. // All tests in this suite will use the compiled library. require(['/base/dist/shaka-player.ui.js'], (shakaModule) => { - compiledShaka = shakaModule; - compiledShaka.net.NetworkingEngine.registerScheme( - 'test', shaka.test.TestScheme.plugin); - compiledShaka.media.ManifestParser.registerParserByMime( - 'application/x-test-manifest', - shaka.test.TestScheme.ManifestParser.factory); + try { + compiledShaka = shakaModule; + compiledShaka.net.NetworkingEngine.registerScheme( + 'test', shaka.test.TestScheme.plugin); + compiledShaka.media.ManifestParser.registerParserByMime( + 'application/x-test-manifest', + shaka.test.TestScheme.ManifestParser.factory); - loaded.resolve(); + loaded.resolve(); + + // We need to catch thrown exceptions here to propertly report errors + // in the registration process above. + // eslint-disable-next-line no-restricted-syntax + } catch (error) { + loaded.reject('Failed to register with compiled player.'); + shaka.log.error('Error registering with compiled player.', error); + } }, (error) => { loaded.reject('Failed to load compiled player.'); shaka.log.error('Error loading compiled player.', error);