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: support esbuild #166

Closed
wants to merge 4 commits into from
Closed

fix: support esbuild #166

wants to merge 4 commits into from

Conversation

lisonge
Copy link

@lisonge lisonge commented Oct 18, 2021

fix the following bug

npm i esbuild node-html-parser typescript
echo 'import { parse, HTMLElement } from "node-html-parser";console.log({ parse, HTMLElement });' > test.ts
npx esbuild test.ts --bundle --outfile=test.js
node test.js

will be output { parse:undefined, HTMLElement:undefined }

@nonara
Copy link
Collaborator

nonara commented Oct 19, 2021

Thanks for the submission!

In order to use as esm your package.json file needs to have "type": "module" set. Please let me know if that solves the issue.

@lisonge
Copy link
Author

lisonge commented Oct 19, 2021

your packge default export is https://github.com/taoqf/node-html-parser/blob/main/src/index.ts#L3 is a function

export { default as parse, default } from './parse';

but in https://github.com/taoqf/node-html-parser/blob/main/esm/index.js#L1

import nhp from '../dist/index.js'

and at dist/index.js#L5

Object.defineProperty(exports, "__esModule", { value: true });

and at #L13

Object.defineProperty(exports, "default", { enumerable: true, get: function () { return __importDefault(parse_1).default; } });

so esbuild will use module.exports.default as import nhp from '../dist/index.js'

so nhp is parse function, not module.exports

@lisonge
Copy link
Author

lisonge commented Oct 19, 2021

Thanks for the submission!

In order to use as esm your package.json file needs to have "type": "module" set. Please let me know if that solves the issue.

I need run node-html-parser in cloudflare workers, without package.json, it is not node runtime
so I use esbuild pack my code to a single file. currently I fix this with patches in my project
@nonara

@nonara
Copy link
Collaborator

nonara commented Oct 19, 2021

your packge default export is https://github.com/taoqf/node-html-parser/blob/main/src/index.ts#L3 is a function

The issue is that package exports are handled differently between ESM and CommonJS.

import nhp from 'node-html-parser' in CommonJS, means import module.exports.default and name it nhp.

In ESM, the same statement means: import the entire module namespace and name it nhp.

Our structure for ESM support follows the proper convention and is not buggy. You can see more detail in this PR:

I need run node-html-parser in cloudflare workers, without package.json

It looks like you don't actually need to be using ESM. If you build to commonJS, you should be fine.

I tried your exact reproduction, and I don't actually experience any issue:

npm i esbuild node-html-parser typescript
echo 'import { parse, HTMLElement } from "node-html-parser";console.log({ parse, HTMLElement });' > test.ts
npx esbuild test.ts --bundle --outfile=test.js
node test.js
{
  parse: [Function: parse],
  HTMLElement: [class HTMLElement extends Node]
}

Do you have a tsconfig.json you can share? If you can setup a repository which can reproduce the issue you're having, I'll take a look and see if I can help.

@lisonge
Copy link
Author

lisonge commented Oct 20, 2021

Do you have a tsconfig.json you can share? If you can setup a repository which can reproduce the issue you're having, I'll take a look and see if I can help.

tsconfig.json in my project https://github.com/lisonge/visit-counter.git

this project will show a bug, If you don't modify node_modules/node-html-parser/esm/index.js

you see bug by following shell

npx esbuild test/test_import.ts --bundle --outfile=dist/test_import.js
node dist/test_import.js

@lisonge
Copy link
Author

lisonge commented Oct 20, 2021

I tried your exact reproduction, and I don't actually experience any issue:

my shell Log

❯ cat typescript
Script started on 2021-10-20 10:26:37+08:00 [TERM="xterm-256color" TTY="/dev/pts/0" COLUMNS="120" LINES="30"]
❯ ls -a .
.  ..  install.sh  typescript
❯ cat install.sh
npm i esbuild node-html-parser typescript
echo 'import { parse, HTMLElement } from "node-html-parser";console.log({ parse, HTMLElement });' > test.ts
npx esbuild test.ts --bundle --outfile=test.js
node test.js

❯ ./install.sh

> esbuild@0.13.8 postinstall /home/lisonge/project/test/node_modules/esbuild
> node install.js

npm WARN saveError ENOENT: no such file or directory, open '/home/lisonge/project/test/package.json'
npm notice created a lockfile as package-lock.json. You should commit this file.
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: esbuild-freebsd-64@0.13.8 (node_modules/esbuild/node_modules/esbuild-freebsd-64):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for esbuild-freebsd-64@0.13.8: wanted {"os":"freebsd","arch":"x64"} (current: {"os":"linux","arch":"x64"})
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: esbuild-freebsd-arm64@0.13.8 (node_modules/esbuild/node_modules/esbuild-freebsd-arm64):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for esbuild-freebsd-arm64@0.13.8: wanted {"os":"freebsd","arch":"arm64"} (current: {"os":"linux","arch":"x64"})
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: esbuild-linux-arm@0.13.8 (node_modules/esbuild/node_modules/esbuild-linux-arm):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for esbuild-linux-arm@0.13.8: wanted {"os":"linux","arch":"arm"} (current: {"os":"linux","arch":"x64"})
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: esbuild-darwin-64@0.13.8 (node_modules/esbuild/node_modules/esbuild-darwin-64):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for esbuild-darwin-64@0.13.8: wanted {"os":"darwin","arch":"x64"} (current: {"os":"linux","arch":"x64"})
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: esbuild-android-arm64@0.13.8 (node_modules/esbuild/node_modules/esbuild-android-arm64):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for esbuild-android-arm64@0.13.8: wanted {"os":"android","arch":"arm64"} (current: {"os":"linux","arch":"x64"})
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: esbuild-linux-32@0.13.8 (node_modules/esbuild/node_modules/esbuild-linux-32):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for esbuild-linux-32@0.13.8: wanted {"os":"linux","arch":"ia32"} (current: {"os":"linux","arch":"x64"})
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: esbuild-linux-mips64le@0.13.8 (node_modules/esbuild/node_modules/esbuild-linux-mips64le):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for esbuild-linux-mips64le@0.13.8: wanted {"os":"linux","arch":"mips64el"} (current: {"os":"linux","arch":"x64"})
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: esbuild-darwin-arm64@0.13.8 (node_modules/esbuild/node_modules/esbuild-darwin-arm64):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for esbuild-darwin-arm64@0.13.8: wanted {"os":"darwin","arch":"arm64"} (current: {"os":"linux","arch":"x64"})
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: esbuild-linux-arm64@0.13.8 (node_modules/esbuild/node_modules/esbuild-linux-arm64):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for esbuild-linux-arm64@0.13.8: wanted {"os":"linux","arch":"arm64"} (current: {"os":"linux","arch":"x64"})
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: esbuild-linux-ppc64le@0.13.8 (node_modules/esbuild/node_modules/esbuild-linux-ppc64le):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for esbuild-linux-ppc64le@0.13.8: wanted {"os":"linux","arch":"ppc64"} (current: {"os":"linux","arch":"x64"})
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: esbuild-netbsd-64@0.13.8 (node_modules/esbuild/node_modules/esbuild-netbsd-64):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for esbuild-netbsd-64@0.13.8: wanted {"os":"netbsd","arch":"x64"} (current: {"os":"linux","arch":"x64"})
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: esbuild-openbsd-64@0.13.8 (node_modules/esbuild/node_modules/esbuild-openbsd-64):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for esbuild-openbsd-64@0.13.8: wanted {"os":"openbsd","arch":"x64"} (current: {"os":"linux","arch":"x64"})
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: esbuild-sunos-64@0.13.8 (node_modules/esbuild/node_modules/esbuild-sunos-64):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for esbuild-sunos-64@0.13.8: wanted {"os":"sunos","arch":"x64"} (current: {"os":"linux","arch":"x64"})
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: esbuild-windows-32@0.13.8 (node_modules/esbuild/node_modules/esbuild-windows-32):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for esbuild-windows-32@0.13.8: wanted {"os":"win32","arch":"ia32"} (current: {"os":"linux","arch":"x64"})
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: esbuild-windows-64@0.13.8 (node_modules/esbuild/node_modules/esbuild-windows-64):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for esbuild-windows-64@0.13.8: wanted {"os":"win32","arch":"x64"} (current: {"os":"linux","arch":"x64"})
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: esbuild-windows-arm64@0.13.8 (node_modules/esbuild/node_modules/esbuild-windows-arm64):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for esbuild-windows-arm64@0.13.8: wanted {"os":"win32","arch":"arm64"} (current: {"os":"linux","arch":"x64"})
npm WARN enoent ENOENT: no such file or directory, open '/home/lisonge/project/test/package.json'
npm WARN test No description
npm WARN test No repository field.
npm WARN test No README data
npm WARN test No license field.

+ node-html-parser@5.0.0
+ typescript@4.4.4
+ esbuild@0.13.8
added 14 packages from 7 contributors and audited 30 packages in 2.238s

8 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities



   ╭───────────────────────────────────────────────────────────────╮
   │                                                               │
   │      New major version of npm available! 6.14.15 → 8.1.0      │
   │   Changelog: https://github.com/npm/cli/releases/tag/v8.1.0   │
   │               Run npm install -g npm to update!               │
   │                                                               │
   ╰───────────────────────────────────────────────────────────────╯


  test.js  322.4kb

⚡ Done in 18ms
{ parse: undefined, HTMLElement: undefined }
❯ exit

Script done on 2021-10-20 10:27:04+08:00 [COMMAND_EXIT_CODE="0"]

test.js here https://gist.github.com/lisonge/bfb21198fa027fa285e7016c0672abbf#file-test-js-L5086

@lisonge
Copy link
Author

lisonge commented Oct 20, 2021

It looks like you don't actually need to be using ESM. If you build to commonJS, you should be fine.

i try --platform=node, but same error

❯ npx esbuild test.ts --bundle --outfile=test.js --platform=node

  test.js  313.1kb

⚡ Done in 19ms
❯ node test.js
{ parse: undefined, HTMLElement: undefined }

@lisonge lisonge closed this Oct 20, 2021
@lisonge lisonge reopened this Oct 20, 2021
@nonara
Copy link
Collaborator

nonara commented Oct 20, 2021

Ok. I cloned your repo and had a look. A few observations:

  1. Something is going wrong with your build
  2. Looks like you don't need to use --platform=node

It works for me with both platform=node and otherwise, using your exact repository.

$ npx esbuild test/test_import.ts --bundle --outfile=dist/test_import.js

  dist\test_import.js  300.2kb

Done in 55ms

$ node dist/test_import.js
{
  parse: [Function: parse],
  HTMLElement: [class HTMLElement extends Node]
}

$ npx esbuild test/test_import.ts --bundle --outfile=dist/test_import.js --platform=node

  dist\test_import.js  310.9kb

Done in 115ms

$ node dist/test_import.js
{ parse: [Function: parse2], HTMLElement: [Function: HTMLElement3] }

I recommend the following:

  1. Uninstall your global version of esbuild (if one is installed)
  2. Ensure pnpm cache is cleared (global and local, if applicable)
  3. Delete pnpm-lock.yaml in your repo
  4. Make sure your package.json specifies the latest version of esbuild
  5. Run pnpm install
  6. Directly execute esbuild from node_modules (without npx) and check the output

If there is an issue beyond that, it's with esbuild. I've confirmed and it's pulling in the proper source code from the commonJS location, so this is not an issue with our ESM wrapper.

Good luck with it! I hope you get it sorted out.

@nonara nonara closed this Oct 20, 2021
@lisonge
Copy link
Author

lisonge commented Oct 21, 2021

i try run shell code at github actions ubuntu-latest
https://github.com/lisonge/test-cli/runs/3958824691?check_suite_focus=true#step:10:1
it still output

{ parse: undefined, HTMLElement: undefined }

can you show dist/test_import.js on your computer?

@lisonge
Copy link
Author

lisonge commented Oct 25, 2021

@nonara
hello, can you show dist/test_import.js on your computer to me?

@nonara
Copy link
Collaborator

nonara commented Oct 25, 2021

Here's my file: https://gist.github.com/nonara/67ef1505a98a2c9ad02bf4ab06f0b183

I'd love to be able to help you track this out, but unfortunately I'm swamped. With that said, hopefully this will help point you in the right direction:

  • It builds properly for me with both --platform=node and without — on Windows

  • Looking at your output and mine shows that esbuild is failing somewhere

  • This is probably related to how esbuild handles the exports field, as the comment output is also wrong:

    Note that even though it works for me, the comment still says this:
    // ../node_modules/node-html-parser/dist/esm/nodes/node.js

    ^ That is not an actual file path. This suggests that esbuild isn't handling the package.sjon exports field correctly, which is a common issue. Handling ESM is not easy, and many libraries break down when given ESM. However, in this case, esbuild should be importing directly from commonjs, which it appears that it is for me. It looks like it is for you, but it's still somehow breaking.

Given the discrepancy between your build failing and mine not, my best guess is that there is an issue between windows and unix.

Recommended steps:

  • Try running GH actions using windows as your OS. If it works, you'll know it's related to OS
  • When you've gathered more info, file a ticket with esbuild and show them the output

Again, I've confirmed that this is not an issue with our esm wrapper, as we followed the proper convention, and it works in both commonjs and esm environments.

Good luck with it, and I hope you get it sorted out!

@lisonge
Copy link
Author

lisonge commented Oct 26, 2021

I think the reason should be this

because https://github.com/taoqf/node-html-parser/blob/main/tsconfig.json#L24

"esModuleInterop": true,

generate https://cdn.jsdelivr.net/npm/node-html-parser@5.0.0/dist/index.js#L5

Object.defineProperty(exports, "__esModule", { value: true });

this line code mark current file is cjs that compiled from esm, it can help keep connection between es modules in cjs runtime

when esm import cjs in nodejs runtime, it doesn't depend on __esModule property,
so it will use exports instead of exports['default']

but esbuild will depend on __esModule property , so it get exports['default']

and when i change Object.defineProperty(exports, "__esModule", { value: true }); to Object.defineProperty(exports, "__esModule", { value: false }); , the [code by esbuild pack] is work

for your this

Note that even though it works for me, the comment still says this:
// ../node_modules/node-html-parser/dist/esm/nodes/node.js

I can't reproduce it at the moment, so i do not know

@nonara
Copy link
Collaborator

nonara commented Oct 26, 2021

This is standard output. File an issue with esbuild.

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.

None yet

2 participants