From 35735a22f76d8eb874242b85ebcaa4b88950e3cb Mon Sep 17 00:00:00 2001 From: Christopher Allen Date: Thu, 12 Jan 2023 12:45:37 +0000 Subject: [PATCH 1/3] 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. --- core/utils/xml.ts | 51 ++++++++++++++++++++++++++---------- scripts/package/node/core.js | 7 ++--- 2 files changed, 39 insertions(+), 19 deletions(-) diff --git a/core/utils/xml.ts b/core/utils/xml.ts index 7e701d12e30..b84b0294576 100644 --- a/core/utils/xml.ts +++ b/core/utils/xml.ts @@ -16,19 +16,42 @@ goog.declareModuleId('Blockly.utils.xml'); /** - * Namespace for Blockly's XML. + * 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 dependencies. 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 should be supplied and sourced + * from the same JSDOM instance. * - * @alias Blockly.utils.xml.NAME_SPACE + * @param dependencies Options object contianing dependencies to set. */ -export const NAME_SPACE = 'https://developers.google.com/blockly/xml'; +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); +} /** - * 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. + * Namespace for Blockly's XML. + * + * @alias Blockly.utils.xml.NAME_SPACE */ -let xmlDocument: Document = globalThis['document']; +export const NAME_SPACE = 'https://developers.google.com/blockly/xml'; /** * Get the document object to use for XML serialization. @@ -37,17 +60,17 @@ let xmlDocument: Document = globalThis['document']; * @alias Blockly.utils.xml.getDocument */ export function getDocument(): Document { - return xmlDocument; + 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. * @alias Blockly.utils.xml.setDocument */ -export function setDocument(document: Document) { - xmlDocument = document; +export function setDocument(xmlDocument: Document) { + document = xmlDocument; } /** @@ -58,7 +81,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 +92,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 28c1b3c9091..d1ec5d05dff 100644 --- a/scripts/package/node/core.js +++ b/scripts/package/node/core.js @@ -18,9 +18,6 @@ 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); + Blockly.utils.xml.injectDependencies({document: window.document}); } From 678643a4e4f24012c36d45e463252b43f63ab7cc Mon Sep 17 00:00:00 2001 From: Christopher Allen Date: Thu, 12 Jan 2023 12:57:20 +0000 Subject: [PATCH 2/3] 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. --- core/utils/xml.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/core/utils/xml.ts b/core/utils/xml.ts index b84b0294576..6576c234fd6 100644 --- a/core/utils/xml.ts +++ b/core/utils/xml.ts @@ -14,6 +14,8 @@ 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 @@ -57,9 +59,11 @@ export const NAME_SPACE = 'https://developers.google.com/blockly/xml'; * 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 { + deprecation.warn('Blockly.utils.xml.getDocument', 'version 9', 'version 10'); return document; } @@ -67,9 +71,11 @@ export function getDocument(): Document { * Get the document object to use for XML serialization. * * @param xmlDocument The document object to use. + * @deprecated No longer provided by Blockly. * @alias Blockly.utils.xml.setDocument */ export function setDocument(xmlDocument: Document) { + deprecation.warn('Blockly.utils.xml.setDocument', 'version 9', 'version 10'); document = xmlDocument; } From 65cba8b30e7e80297607fced214635a3c9bc9a80 Mon Sep 17 00:00:00 2001 From: Christopher Allen Date: Thu, 12 Jan 2023 18:08:59 +0000 Subject: [PATCH 3/3] fix: Corrections for comments on PR #6764. --- core/utils/xml.ts | 21 ++++++++++++++++----- scripts/package/node/core.js | 1 - 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/core/utils/xml.ts b/core/utils/xml.ts index 6576c234fd6..c3371299287 100644 --- a/core/utils/xml.ts +++ b/core/utils/xml.ts @@ -27,13 +27,24 @@ import * as deprecation from './deprecation.js'; let {document, DOMParser, XMLSerializer} = globalThis; /** - * Inject dependencies. Used by the Node.js wrapper for Blockly (see + * 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 should be supplied and sourced - * from the same JSDOM instance. + * 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 contianing dependencies to set. + * @param dependencies Options object containing dependencies to set. */ export function injectDependencies(dependencies: { document?: Document, diff --git a/scripts/package/node/core.js b/scripts/package/node/core.js index d1ec5d05dff..aead71a0324 100644 --- a/scripts/package/node/core.js +++ b/scripts/package/node/core.js @@ -19,5 +19,4 @@ if (typeof globalThis.document !== 'object') { const {JSDOM} = require('jsdom'); const {window} = new JSDOM(``); Blockly.utils.xml.injectDependencies(window); - Blockly.utils.xml.injectDependencies({document: window.document}); }