Skip to content

Commit

Permalink
fix(deps): Don't use global variables for jsdom injection in `scripts…
Browse files Browse the repository at this point in the history
…/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.
  • Loading branch information
cpcallen authored Jan 12, 2023
1 parent 3cf0663 commit cd57e74
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 19 deletions.
68 changes: 54 additions & 14 deletions core/utils/xml.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,40 +14,80 @@
import * as goog from '../../closure/goog/goog.js';
goog.declareModuleId('Blockly.utils.xml');

import * as deprecation from './deprecation.js';


/**
* Namespace for Blockly's XML.
*
* @alias Blockly.utils.xml.NAME_SPACE
* 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.
*/
export const NAME_SPACE = 'https://developers.google.com/blockly/xml';
let {document, DOMParser, XMLSerializer} = globalThis;

/**
* 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
* 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.
*/
let xmlDocument: Document = globalThis['document'];
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.
*
* @alias Blockly.utils.xml.NAME_SPACE
*/
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 {
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;
}

/**
Expand All @@ -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);
}

/**
Expand All @@ -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);
}

/**
Expand Down
6 changes: 1 addition & 5 deletions scripts/package/node/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,5 @@
if (typeof globalThis.document !== 'object') {
const {JSDOM} = require('jsdom');
const {window} = new JSDOM(`<!DOCTYPE html>`);
globalThis.DOMParser = window.DOMParser;
globalThis.XMLSerializer = window.XMLSerializer;
const xmlDocument = Blockly.utils.xml.textToDomDocument(
`<xml xmlns="${Blockly.utils.xml.NAME_SPACE}"></xml>`);
Blockly.utils.xml.setDocument(xmlDocument);
Blockly.utils.xml.injectDependencies(window);
}

0 comments on commit cd57e74

Please sign in to comment.