From cd57e74d1ac434ec3ca3b5b611eac673591fb317 Mon Sep 17 00:00:00 2001 From: Christopher Allen Date: Thu, 12 Jan 2023 23:31:53 +0000 Subject: [PATCH] fix(deps): Don't use global variables for jsdom injection in `scripts/package/node/core.js` and `core/utils/xml.ts` (#6764) * fix(node): Don't use global variables for jsdom injection Introduce a (hopefully generally applicable) mechanism for injecting dependencies into modules, specifically in this case to inject required bits of JSDOM's Window and Document implementations into core/utils/xml.js when running in node.js or other environments lacking a DOM. The injectDependencies function uses an options object to facilitate optionally injecting multiple named dependencies at the same time. Rename the xmlDocument local variable back to document (was renamed in #5461) so that the name used in this module corresponds to the usual global variable it replaces. Change the injection in scripts/package/node/core.js to use injectDependencies instead of setXmlDocument + global variables; also eliminate apparently-unnecessary creation of a special Document instance, using the default one supplied by jsdom instead. Fixes #6725. * deprecate(xml): Deprecate getXmlDocument and setXmlDocument Mark getXmlDocument and setXmlDocument as @deprecated, with suitable calls to deprecation.warn(). There are no remaining callers to either function within core - setXmlDocument was only used by the node.js wrapper, and and apparently getXmlDocument was never used AFAICT - and we do not anticipate that either were used by external developers. * fix: Corrections for comments on PR #6764. --- core/utils/xml.ts | 68 ++++++++++++++++++++++++++++-------- scripts/package/node/core.js | 6 +--- 2 files changed, 55 insertions(+), 19 deletions(-) diff --git a/core/utils/xml.ts b/core/utils/xml.ts index 7e701d12e..c33712992 100644 --- a/core/utils/xml.ts +++ b/core/utils/xml.ts @@ -14,6 +14,50 @@ import * as goog from '../../closure/goog/goog.js'; goog.declareModuleId('Blockly.utils.xml'); +import * as deprecation from './deprecation.js'; + + +/** + * Injected dependencies. By default these are just (and have the + * same types as) the corresponding DOM Window properties, but the + * Node.js wrapper for Blockly (see scripts/package/node/core.js) + * calls injectDependencies to supply implementations from the jsdom + * package instead. + */ +let {document, DOMParser, XMLSerializer} = globalThis; + +/** + * Inject implementations of document, DOMParser and/or XMLSerializer + * to use instead of the default ones. + * + * Used by the Node.js wrapper for Blockly (see + * scripts/package/node/core.js) to supply implementations from the + * jsdom package instead. + * + * While they may be set individually, it is normally the case that + * all three will be sourced from the same JSDOM instance. They MUST + * at least come from the same copy of the jsdom package. (Typically + * this is hard to avoid satsifying this requirement, but it can be + * inadvertently violated by using webpack to build multiple bundles + * containing Blockly and jsdom, and then loading more than one into + * the same JavaScript runtime. See + * https://github.com/google/blockly-samples/pull/1452#issuecomment-1364442135 + * for an example of how this happened.) + * + * @param dependencies Options object containing dependencies to set. + */ +export function injectDependencies(dependencies: { + document?: Document, + DOMParser?: typeof DOMParser, + XMLSerializer?: typeof XMLSerializer, +}) { + ({ + // Default to existing value if option not supplied. + document = document, + DOMParser = DOMParser, + XMLSerializer = XMLSerializer, + } = dependencies); +} /** * Namespace for Blockly's XML. @@ -22,32 +66,28 @@ goog.declareModuleId('Blockly.utils.xml'); */ export const NAME_SPACE = 'https://developers.google.com/blockly/xml'; -/** - * The Document object to use. By default this is just document, but - * the Node.js build of Blockly (see scripts/package/node/core.js) - * calls setDocument to supply a Document implementation from the - * jsdom package instead. - */ -let xmlDocument: Document = globalThis['document']; - /** * Get the document object to use for XML serialization. * * @returns The document object. + * @deprecated No longer provided by Blockly. * @alias Blockly.utils.xml.getDocument */ export function getDocument(): Document { - return xmlDocument; + deprecation.warn('Blockly.utils.xml.getDocument', 'version 9', 'version 10'); + return document; } /** * Get the document object to use for XML serialization. * - * @param document The document object to use. + * @param xmlDocument The document object to use. + * @deprecated No longer provided by Blockly. * @alias Blockly.utils.xml.setDocument */ -export function setDocument(document: Document) { - xmlDocument = document; +export function setDocument(xmlDocument: Document) { + deprecation.warn('Blockly.utils.xml.setDocument', 'version 9', 'version 10'); + document = xmlDocument; } /** @@ -58,7 +98,7 @@ export function setDocument(document: Document) { * @alias Blockly.utils.xml.createElement */ export function createElement(tagName: string): Element { - return xmlDocument.createElementNS(NAME_SPACE, tagName); + return document.createElementNS(NAME_SPACE, tagName); } /** @@ -69,7 +109,7 @@ export function createElement(tagName: string): Element { * @alias Blockly.utils.xml.createTextNode */ export function createTextNode(text: string): Text { - return xmlDocument.createTextNode(text); + return document.createTextNode(text); } /** diff --git a/scripts/package/node/core.js b/scripts/package/node/core.js index 28c1b3c90..aead71a03 100644 --- a/scripts/package/node/core.js +++ b/scripts/package/node/core.js @@ -18,9 +18,5 @@ if (typeof globalThis.document !== 'object') { const {JSDOM} = require('jsdom'); const {window} = new JSDOM(``); - globalThis.DOMParser = window.DOMParser; - globalThis.XMLSerializer = window.XMLSerializer; - const xmlDocument = Blockly.utils.xml.textToDomDocument( - ``); - Blockly.utils.xml.setDocument(xmlDocument); + Blockly.utils.xml.injectDependencies(window); }