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.

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

fix: ensure toNodeList uses doc var #134

Closed
wants to merge 1 commit into from

Conversation

iamstuartwilson
Copy link

@iamstuartwilson iamstuartwilson commented Nov 29, 2024

Overview

The latest release has broken our esbuild compilation where we use this package via the esbuild-html-plugin, running in Node.js. As such this fix should ensure global.document is used in the recently added toNodeList method and prevent the runtime error we're seeing:

node_modules/nwsapi/src/nwsapi.js:215:20: ERROR: [plugin: esbuild-html-plugin] document is not defined

Closes #135
Closes #136

@manu78
Copy link

manu78 commented Nov 29, 2024

please

Copy link

@nuragic nuragic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🫶

@blackrabbit99
Copy link

I believe you missed some tests in your PR :(

@zorcec
Copy link

zorcec commented Nov 29, 2024

Please hurry up! This seams to break the jsdom <3

@arqex
Copy link

arqex commented Nov 29, 2024

That's it, jsdom install this update automatically and it breaks. I've tried to make the change in this PR manually and I'm getting another error. NodeList is undefined.

There are some instanceof trying to use NodeList class, but it's undefined, the same way than document was undefined.

@dm-gc
Copy link

dm-gc commented Nov 29, 2024

Where were the test cases for this particular scenario, when you needed them the most? :'(

@dperini
Copy link
Owner

dperini commented Nov 29, 2024

@ALL pushed change in 2.2.15
New toNodeList() method functionality disabled via Config object, Config.ANODELIST: false,

@mjouneh
Copy link

mjouneh commented Nov 29, 2024

Any update on it? it's break the Jsdom

@dmamyk
Copy link

dmamyk commented Nov 29, 2024

Please fix ASAP

@Filipoliko
Copy link

@ALL pushed change in 2.2.15 New toNodeList() method functionality disabled via Config object, Config.ANODELIST: false,

Not sure, if it is supposed to fix the issue with jsdom, but with latest version of nwsapi I am still getting this error while running jest tests with jsdom.

  ● Test suite failed to run

    The error below may be caused by using the wrong test environment, see https://jestjs.io/docs/configuration#testenvironment-string.
    Consider using the "jsdom" test environment.
    
    ReferenceError: document is not defined

      at node_modules/nwsapi/src/nwsapi.js:215:21
      at Factory (node_modules/nwsapi/src/nwsapi.js:245:6)

@mjouneh
Copy link

mjouneh commented Nov 29, 2024

For me as well, version 2.2.15
ReferenceError: document is not defined at node_modules/nwsapi/src/nwsapi.js:215:21\

@iamstuartwilson
Copy link
Author

@dperini any reason this wouldn't address the issue. global.document is used elsewhere and isn't causing build issues in Node.js, so I assume this would work, but perhaps it's a naive approach?

@melenudo
Copy link

melenudo commented Nov 29, 2024

@dperini any reason this wouldn't address the issue. global.document is used elsewhere and isn't causing build issues in Node.js, so I assume this would work, but perhaps it's a naive approach?

I'm agree. This library can be used in Node.js environment (jsdom for example) and you can't use document, you must use global.document.

@iamstuartwilson
Copy link
Author

@blackrabbit99 feel free to add tests if you know how. I see a lot of directories without updates for 7 years, so I'm not sure. Perhaps @dperini can comment on this.

@melenudo
Copy link

Setting Config.ANODELIST to false also fixes the problem but 2.2.15 still has Config.ANODELIST to true (in npm repository).

@melenudo
Copy link

Setting Config.ANODELIST to false also fixes the problem but 2.2.15 still has Config.ANODELIST to true (in npm repository).

Sorry, 2.2.15 has Config.ANODELIST to false, but doesn't resolve the problem.

@melenudo
Copy link

That's because document.createFragment is always called on bootstrap (ignoring the Config property):
Captura de pantalla 2024-11-29 a las 17 15 41

@dperini
Copy link
Owner

dperini commented Nov 29, 2024

https://github.com/ALL pushed change in 2.2.16
createDocumentFragment() invoked on "global.document".

@iamstuartwilson
Copy link
Author

Open source at its finest!

unstubbable added a commit to vercel/next.js that referenced this pull request Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

in Version 2.2.14 is NodeList missing Last version is breaking my tests