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] use file:// URLs for dynamic import() #571

Merged
merged 1 commit into from Nov 14, 2021
Merged

[Fix] use file:// URLs for dynamic import() #571

merged 1 commit into from Nov 14, 2021

Conversation

vweevers
Copy link
Contributor

Alternative to #559.

This is required on Windows if the argument passed to import() is an absolute path. Without it tape test.js fails if test.js is ESM.

Covered by existing tests: node test/import.js fails without this (as well as some other tests but those seem less relevant to the issue at hand).

See nodejs/node@a084632

This is required on Windows if the argument passed to `import()` is
an absolute path. Without it `tape test.js` fails if test.js is ESM.

Covered by existing tests: `node test/import.js` fails without this.

See nodejs/node@a084632
vweevers added a commit to vweevers/hallmark that referenced this pull request Nov 13, 2021
CommonJS modules can no longer `require('hallmark')`. They must use
ESM themselves, or a dynamic `import()`. For further guidance please
see https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c

As for the `hallmark` CLI: in Node.js versions older than 14 it is
now a noop.

Needed for #80.
Depends on tape-testing/tape#571
@codecov-commenter
Copy link

codecov-commenter commented Nov 13, 2021

Codecov Report

Merging #571 (d487add) into master (e41763f) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #571   +/-   ##
=======================================
  Coverage   96.49%   96.49%           
=======================================
  Files           4        4           
  Lines         627      627           
  Branches      145      145           
=======================================
  Hits          605      605           
  Misses         22       22           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e41763f...d487add. Read the comment docs.

@ljharb
Copy link
Collaborator

ljharb commented Nov 13, 2021

@vweevers and with this, do all the windows tests pass? or are there still some failures left.

@vweevers
Copy link
Contributor Author

All but one (test/async-await.js):

Click to expand
$ npx tap test/*.js
test/add-subtest-async.js ............................. 2/2
test/anonymous-fn.js .................................. 1/1
test/array.js ......................................... 1/1
test/async-await.js ................................. 23/24
  sync-error
  not ok should be equivalent
    +++ found
    --- wanted
     [
    -  "$TEST/async-await/sync-error.js:7"
    -  "    throw new Error('oopsie');"
    -  "    ^"
"                              error.js:7
"                                 e');
"
"
"
"   +  "    at Test.myTest ($TEST/async-await/sync-error.js:$LINE:$COL)
"      "    at Test.bound [as _cb] ($TAPE/lib/test.js:$LINE:$COL)
"                 t.run ($TAPE/lib/test.js:$LINE:$COL)
"      "    at Test.bound [as run] ($TAPE/lib/test.js:$LINE:$COL)
"      "    at processImmediate (node:internal/timers:$LINE:$COL)
       ""
    -  "Error: oopsie"
    -  "    at Test.myTest ($TEST/async-await/sync-error.js:$LINE:$COL)"
    -  "    at Test.bound [as _cb] ($TAPE/lib/test.js:$LINE:$COL)"
    -  "    at Test.run ($TAPE/lib/test.js:$LINE:$COL)"
    -  "    at Test.bound [as run] ($TAPE/lib/test.js:$LINE:$COL)"
    -  ""
     ]
    at:
      line: 206
      column: 11
      file: test/async-await.js
    stack: |
      test/async-await.js:206:11
      ChildProcess.<anonymous> (test/common.js:102:9)
      ChildProcess.emit (node:events:394:28)
      Process.ChildProcess._handle.onexit (node:internal/child_process:290:12)
    source: |
      t.same(stripFullStack(stderr), [].concat(

test/bound.js ......................................... 2/2
test/child_ordering.js .............................. 14/14
test/circular-things.js ............................... 1/1
test/comment.js ....................................... 7/7
test/common.js ........................................ 0/1
  Skipped: 1
    test/common.js No tests found

test/create_multiple_streams.js ....................... 2/2
test/deep-equal-failure.js ............................ 9/9
test/deep.js ........................................ 14/14
test/default-messages.js .............................. 1/1
test/double_end.js .................................... 2/2
test/edge-cases.js .................................... 1/1
test/end-as-callback.js ............................... 1/1
test/error.js ......................................... 1/1
test/exit.js ........................................ 14/14
test/exposed-harness.js ..1..0
test/exposed-harness.js ............................... 2/2
test/fail.js .......................................... 1/1
test/has spaces.js .................................... 1/1
test/ignore_from_gitignore.js ......................... 0/3
  Skipped: 3
    Should pass with ignoring
    Should pass
    Should fail when ignore file does not exist

test/import.js ...................................... 26/26
test/many.js ...................................... 100/100
test/match.js ......................................... 2/2
test/max_listeners.js ............................... 11/11
test/nested-async-plan-noend.js ....................... 5/5
test/nested-sync-noplan-noend.js ...................... 1/1
test/nested.js ........................................ 1/1 311ms
test/nested2.js ....................................... 1/1
test/no_callback.js ................................... 1/1
test/not-deep-equal-failure.js ........................ 9/9
test/not-equal-failure.js ............................. 3/3
test/numerics.js ...................................... 1/1
test/objectMode.js .................................... 9/9
test/objectModeWithComment.js ......................... 2/2
test/onFailure.js ..................................... 1/1
test/onFinish.js .1..0
test/onFinish.js ...................................... 1/1
test/only-twice.js .................................... 1/1
test/only.js .......................................... 2/2
test/only2.js ......................................... 0/1
  Skipped: 1
    test/only2.js No tests found

test/only3.js ......................................... 0/1
  Skipped: 1
    test/only3.js No tests found

test/only4.js ......................................... 0/1
  Skipped: 1
    test/only4.js No tests found

test/only5.js ......................................... 0/1
  Skipped: 1
    test/only5.js No tests found

test/order.js ......................................... 3/3
test/plan_optional.js ................................. 2/2
test/promise_fail.js .................................. 2/2
test/require.js ....................................... 4/4
test/skip_explanation.js .............................. 1/1
test/skip.js .1..0
test/skip.js .......................................... 1/1
test/stackTrace.js .................................. 31/31
test/subcount.js ...................................... 2/2
test/subtest_and_async.js ............................. 4/4
test/subtest_plan.js .................................. 4/4
test/teardown.js ....1..0
test/teardown.js ...................................... 4/4
test/throws.js ........................................ 1/1
test/timeout.js ....................................... 2/2
test/timeoutAfter.js .................................. 2/2
test/todo_explanation.js .............................. 1/1
test/todo_single.js ................................... 1/1
test/todo.js .......................................... 1/1
test/too_many.js ...................................... 1/1
test/undef.js ......................................... 1/1
total ............................................. 345/354


  345 passing (11s)
  8 pending
  1 failing

And there's some problem with nyc:

Click to expand
$ npm run tests-only

> tape@5.3.1 tests-only
> nyc tap test/*.js

node:internal/modules/cjs/loader:936
  throw err;
  ^

Error: Cannot find module 'D:\GitHub\forks\tape\node'
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:933:15)
    at Function.Module._load (node:internal/modules/cjs/loader:778:27)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:79:12)
    at node:internal/main/run_main_module:17:47 {
  code: 'MODULE_NOT_FOUND',
  requireStack: []
}

@ljharb ljharb merged commit d487add into tape-testing:master Nov 14, 2021
@vweevers vweevers deleted the windows-esm branch November 14, 2021 07:48
vweevers added a commit to vweevers/hallmark that referenced this pull request Nov 14, 2021
CommonJS modules can no longer `require('hallmark')`. They must use
ESM themselves, or a dynamic `import()`. For further guidance please
see https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c

As for the `hallmark` CLI: in Node.js versions older than 14 it is
now a noop.

Needed for #80.
Depends on tape-testing/tape#571
ljharb added a commit that referenced this pull request Nov 16, 2021
 - [Fix] use `file://` URLs for dynamic `import()` (#571, #559)
 - [readme] hard wraps bad, soft wraps good
 - [readme] fix markdown; github still has a rendering bug
 - [readme] add badges
 - [meta] Exclude `fs` from browser bundles (#565)
 - [Deps] update `glob`, `string.prototype.trim`
 - [Dev Deps] update `eslint`, `@ljharb/eslint-config`, `safe-publish-latest`, `array.prototype.flatmap`
 - [actions] update codecov uploader
 - [Tests] handle carriage returns in stack traces on Windows
 - [Tests] node 17+ smooshes a version number on the end of the stack trace
 - [Tests] handle v8 6.9 changing an error message
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

3 participants