Use dynamic import for node util

charlag commented 4 years ago

Description

This commit replaces require() with import().

Motivation and Context

With this change ospec is able to run in both es6 and commonjs modes of node. require() is not allowed in es6 mode which is a problem for apps which build targets esm.

How Has This Been Tested?

npm test

Types of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)
[ ] Documentation change

Checklist:

[x] My code follows the code style of this project.
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[x] I have read all applicable contributing documents.
[ ] I have added tests to cover my changes.
[x] All new and existing tests passed.
[x] I have updated the change log (if applicable).

dead-claudia commented 4 years ago

Have you confirmed this to work in files where it's all synchronous like this?

const o = require("ospec")

o.spec("test", () => {
    o("it works", () => {
        o(1).equals(0)
    })
})

o.run()

Edit: should be checking for failures, not successes

charlag commented 3 years ago

Well, I'm not sure how to proceed here. I tried to make the whole serialize async but then its users expect it to update the result. I would have to make even more internals async and I don't even know where to start

charlag commented 3 years ago

I noticed that it anyway always runs spec with nextTickish so I wonder why it might not work. Maybe I should wait for that promise and then run spec?

charlag commented 3 years ago

@isiahmeadows I made it work by replacing nextTickish with waiting for promise which is also async and is exactly what I need. It was indeed quite trivial change. I've included the file for testing import but I don't know how to test it as the test suite will already import promise before running the test... chicken and egg.

charlag commented 3 years ago

(failing test in the CI is the test I added and it behaves like it should but obviously CI shouldn't fail, feedback welcome)

pygy commented 3 years ago

@charlag ospec.js itself is a CJS module (UMD of sorts, actually, but CJS as far as Node is concerned), it can thus call require() when run in Node...

Even if your project has "type": "module" in the package.json, ospec will be loaded from node_modules and run as CJS.

In what scenario did you experience breakage?

charlag commented 3 years ago

We use rollup/nollup for bundling. Native format for it is ESM and it's also our target format becaue we want to run the same build in browser and in node. After bundling ospec becomes part of some bundle and it not in node_modules. Normally it's fine because plugins wrap commonjs modules to make them work like ESM modules but they miss this require() inside ospec, probably because it's kinda deep and conditional. So the resulting build has ospec code inside of some bundle which is expected to work like ES module but it tries to use require like commonjs module.

There are workarounds for that like copying node_modules/ospec to the build directory and including it manully into webpage for browser instead of relying on bundle but currently we just use fork with this change.

I hope I was able to clarify a little bit.

pygy commented 3 years ago

Oh, and somehow, in that environment, ospec ends up thinking it runs in Node (I suppose that a process shim ends up in you build).

I'd fix this by replacing if (hasProcess) return require("util").inspect(value) with if (typeof require != null) .... It will spare everyone headaches.

@barneycarroll has improvements planned for the browser reports that will be better than util.inspect.

charlag commented 3 years ago

Well it does run in node in half cases, external imports are fne, it's just it's not explicit. Maybe if something was different bundler would rewrite the import as well but I just tried to make it work everywhere.

pygy commented 3 years ago

Oh, you run the bundle in Node...

import() will not work in Node v12 (LTS until 2022, the parser accepts it, but it throws at runtime), and it IIRC will throw a syntax error in IE.

Likewise, IE compat means we can't expect Promise to be generally available.

I think it can be fixed but we'll need try/catch/eval-based shenanigans similar to what's found in bin/ospec.

Something along this (not tested, but you get the idea):

function serialize(value) {
  // no inspect logic for now
  if (value === null || (typeof value === "object" && !(value instanceof Array)) || typeof value === "number") return String(value)
  else if (typeof value === "function") return value.name || "<anonymous function>"
  try {return JSON.stringify(value)} catch (e) {return String(value)}
}

function loadInspect(cb) {
  try {
    // eval is needed because`import()` is a syntax error in older node.js versions (pre 13.2)
    // also some node versions (12.13+ at least) do support the syntax but reject the promise
    // at run time...
    // eslint-disable-next-line no-eval
    eval("import('util')").then(
      ({inspect}) => {serialize = inspect},
      (e) => { // Node v12
        if (e.message.includes('Not supported')) serialize = require("util").inspect
      }
    ).finally(cb))
  } catch(_) {
    nextTickish(cb)
  }
}

And you replace the nextTickish call in o.run with loadInspect.

charlag commented 3 years ago

Ouch I think it depends on node 12 version too but if you really expect people to test in IE then this should not be accepted and doesn't make much sense either.

pygy commented 3 years ago

It is half-supported since IIRC v12.3, but I only care about the latest version of the v12 branch anyway... Don't you think the loadInspect logic above would work for you (you might have missed the edit)?

charlag commented 3 years ago

I think it would work thanks for the proposal, I will try it out later. Not sure if it's worth it tho, it doesn't look that nice but I guess it's up to you.

pygy commented 3 years ago

Yeah, elegance is in the eye of the beholder... We try to keep have the lib broadly compatible, and that means compromising on code neatness. I haven't tested it in ages, but v4.1.1 should still be compatible with IE, and I'd rather not break someone's test suite.

The comment is copied from the binary (and slightly wrong, now that I re-read it), and needs to be amended in this context of course.

pygy commented 2 years ago

@charlag, thanks for the PR, and sorry for the delay. This has been fixed in another manner. I'm going through the backlog, and had forgotten about this PR before fixing it myself.

charlag commented 2 years ago

Hi, no stress, we use a fork for a long time. Version in the repo will just end up not including it but it's fine in most cases.

pygy commented 2 years ago

Rollup was hoisting the require call at the top of the file. I've put it in a try/catch block, and Rollup leaves that alone now. So after rolling-up ospec, you get the default browser behavior. This means it will display deepEquals objects as [Object object] etc..., which is sub-optimal, but at least it doesn't crash upfront.

MithrilJS / ospec