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

JSDOM port #542

Open
wants to merge 9 commits into
base: master
from

Conversation

@MarkTiedemann
Copy link
Contributor

commented Jul 20, 2019

Closes #537.

$ cd jsdom
$ npm install --no-package-lock webpack@4.36.1 webpack-cli@3.3.6 jsdom@15.1.1
$ ./node_modules/.bin/webpack-cli
Hash: 80cb94c06fc1cfb9a48b
Version: webpack 4.36.1
Time: 2422ms
Built at: 07/20/2019 11:51:39 PM
     Asset      Size  Chunks                    Chunk Names
./jsdom.js  3.05 MiB       0  [emitted]  [big]  main
Entrypoint main [big] = ./jsdom.js
 [17] (webpack)/buildin/global.js 472 bytes {0} [built]
[108] (webpack)/buildin/module.js 497 bytes {0} [built]
[231] canvas (ignored) 15 bytes {0} [built]
[440] ./streams (ignored) 15 bytes {0} [built]
[441] ./extend-node (ignored) 15 bytes {0} [built]
[519] util (ignored) 15 bytes {0} [built]
[521] util (ignored) 15 bytes {0} [built]
[562] buffer (ignored) 15 bytes {0} [optional] [built]
[563] crypto (ignored) 15 bytes {0} [optional] [built]
[625] crypto (ignored) 15 bytes {0} [built]
    + 948 hidden modules
$ deno jsdom.ts
Compile file:///Users/marktiedemann/dev/deno_std/jsdom/jsdom.ts
Hello, Deno!

TODOs:

  • Add preliminary types.
  • Add preliminary tests.
  • Write documentation.

Looking for feedback.

cc @ry

jsdom/jsdom.ts Outdated Show resolved Hide resolved
jsdom/jsdom.ts Outdated Show resolved Hide resolved

MarkTiedemann added some commits Jul 22, 2019

@MarkTiedemann MarkTiedemann changed the title WIP: JSDOM port JSDOM port Jul 25, 2019

@MarkTiedemann

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

I think this is ready to be reviewed now, @ry.

One major painpoint, I guess, is that the DOM and JSDOM types are still preliminary. There are also some other future TODOs, for example, investigating whether the bundle size can be improved, testing more of JSDOM's functionality, and rewriting/wrapping some Node specific-parts in JSDOM (for example, JSDOM.fromURL and JSDOM.fromFile both use built-in Node modules). But I think these can be tackled in future PRs.

import { jsdom as instance } from "./vendor/jsdom.js";
import { jsdom } from "./types/jsdom.ts";

export const JSDOM = instance.JSDOM as jsdom.JSDOM;

This comment has been minimized.

Copy link
@j-f1

j-f1 Aug 13, 2019

Another implementation of this that should also work:

export { jsdom as JSDOM } from "./vendor/jsdom.js";
export { jsdom as JSDOM } from "./types/jsdom.ts";

This comment has been minimized.

Copy link
@MarkTiedemann

MarkTiedemann Aug 13, 2019

Author Contributor

Thanks for the suggestion, @j-f1. Unfortunately, this doesn't work on my machine. When I change it, JSDOM can't be imported anymore.

$ deno test.ts
Compile file:///Users/marktiedemann/dev/deno_std/jsdom/mod.ts
error: Uncaught SyntaxError: The requested module './mod.ts' does not provide an export named 'JSDOM'

This comment has been minimized.

Copy link
@j-f1

j-f1 Aug 14, 2019

Well, that’s unexpected. Sorry to send you on a wild goose chase!

This comment has been minimized.

Copy link
@MarkTiedemann

MarkTiedemann Aug 14, 2019

Author Contributor

No worries, @j-f1.

I just tried again, with the same code, and got a different error:

$ deno test.ts
Compile file:///Users/marktiedemann/dev/deno_std/jsdom/mod.ts

error TS2300: Duplicate identifier 'JSDOM'.

► file:///Users/marktiedemann/dev/deno_std/jsdom/mod.ts:1:19

1 export { jsdom as JSDOM } from "./vendor/jsdom.js";
                    ~~~~~

error TS2300: Duplicate identifier 'JSDOM'.

► file:///Users/marktiedemann/dev/deno_std/jsdom/mod.ts:2:19

2 export { jsdom as JSDOM } from "./types/jsdom.ts";
                    ~~~~~


Found 2 errors.

This seems like a more expected error, still unfortunate.

This comment has been minimized.

Copy link
@kitsonk

kitsonk Aug 17, 2019

Contributor

I suspect denoland/deno#2746 will fix this (or at least make it possible).

This comment has been minimized.

Copy link
@MarkTiedemann

MarkTiedemann Aug 17, 2019

Author Contributor

@kitsonk, awesome! As soon as denoland/deno#2746 is released, I will update the types here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.