Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Don't use global variables in scripts/package/node/core.js and core/utils/xml.ts #6725

Closed
cpcallen opened this issue Dec 24, 2022 · 1 comment · Fixed by #6764
Closed

Don't use global variables in scripts/package/node/core.js and core/utils/xml.ts #6725

cpcallen opened this issue Dec 24, 2022 · 1 comment · Fixed by #6764
Assignees
@cpcallen
Labels
issue: bug Describes why the code or behaviour is wrong type: regression

Comments

@cpcallen
Copy link
Contributor

Describe the bug

Due to the use of global variables to pass some (but not all) items obtained from the jsdom package from scripts/package/node/core.js to core/utils/xml.ts, XML serialisation (and therefore Blockly generally) is broken if:

  1. Blockly is being loaded in node.js (or another runtime that does not have a native DOM implementation), and
  2. more than one copy of Blocly is loaded.

Normally modules cannot be loaded more than once, but multiple copies of a module with different paths, or a single copy that is included in more than one (e.g. webpack) bundle, can defeat the usual deduping of loads to satisfy imports and requires.

Specifically, due to this bug, the @blockly/dev-scripts's test script, used by plugin developers (including us), is broken when used to test a project with more than one test/*.mocha.js test harness when using Blockly v9.2.0.

To Reproduce

Steps to reproduce the behavior:

  1. Clone blockly-samples.
  2. Choose any plugin with more than one test script in its test/ drirectory, where at least one of those tests creates a block.
  3. Run npm upgrade blockly to bump blockly to 9.2.0.
  4. Run npm test
  5. Observe test errors.

Expected behaviour

Test should pass; no errors should occur.

Stack Traces

     TypeError: Failed to execute 'serializeToString' on 'XMLSerializer': parameter 1 is not of type 'Node'.
      at Object.exports.convert (webpack-internal:///./node_modules/jsdom/lib/jsdom/living/generated/Node.js:25:9)
      at XMLSerializer.serializeToString (webpack-internal:///./node_modules/jsdom/lib/jsdom/living/generated/XMLSerializer.js:111:23)
      at domToText$$module$build$src$core$utils$xml (webpack-internal:///./node_modules/blockly/blockly_compressed.js:74:333)

Additional context

See this lengthy discussion for more detail about the issue.
@cpcallen cpcallen added issue: bug Describes why the code or behaviour is wrong type: regression issue: triage Issues awaiting triage by a Blockly team member labels Dec 24, 2022
@cpcallen
Copy link
Contributor Author

cpcallen commented Jan 3, 2023

Proposed solution:

In utils/xml.ts

  • Add one or more setters to utils/xml.ts to inject DOMParser, XMLSerializer or maybe the entire jsdom window object, and use the injected versions (defaulting to the browser-provided globals only if no other implementation is injected).
    • Do this in a way that is backward-compatible with the setDocument setter.
    • Consider doing so in a way that obviates the need for setDocument to be used at all, allowing it to be deprecated.
    • Consider marking new setters @internal.

In scripts/package/node/core.js:

  • Call the new setter(s) to inject the jsdom-provided DOMParser and XMLSerializer functions ( or maybe the entire window object) to utils/xml.ts.
  • Continue to set globalThis.DOMParser and globalThis.XMLSerializer, but include a wrapper noting they are deprecated. This is to avoid the change being a breaking change in the (perhaps unlikely) case that someone is using Blockly in node.js and has been relying on the versions of these functions we have been injecting into the global scope.

In a future major version, remove the code that modifies globalThis from scripts/package/node/core.js, and remove the setDocument setter from utils/xml.ts.

@BeksOmega BeksOmega removed the issue: triage Issues awaiting triage by a Blockly team member label Jan 11, 2023
cpcallen added a commit to cpcallen/blockly that referenced this issue Jan 12, 2023
@cpcallen
fix(node): Don't use global variables for jsdom injection
673ea2f 
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 google#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 google#6725.
@cpcallen cpcallen mentioned this issue Jan 12, 2023
Merged
4 tasks
cpcallen added a commit to cpcallen/blockly that referenced this issue Jan 12, 2023
@cpcallen
fix(node): Don't use global variables for jsdom injection
35735a2 
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 google#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 google#6725.
BeksOmega pushed a commit that referenced this issue Jan 12, 2023
@cpcallen
fix(deps): Don't use global variables for jsdom injection in `scripts…
cd57e74 
…/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.
@cpcallen cpcallen self-assigned this Jan 13, 2023
gonfunko pushed a commit that referenced this issue Jan 19, 2023
@cpcallen @gonfunko
fix(deps): Don't use global variables for jsdom injection in `scripts…
f6bde95 
…/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.

(cherry picked from commit cd57e74)
This was referenced Mar 5, 2024
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cpcallen cpcallen

Labels
issue: bug Describes why the code or behaviour is wrong type: regression
Projects
None yet
Milestone
No milestone
2 participants
@cpcallen @BeksOmega