assert is not browser friendly

[JounQin]

Initial checklist

• I read the support docs
• I read the contributing guide
• I agree to follow the code of conduct
• I searched issues and couldn’t find anything (or linked relevant results below)

Affected packages and versions

latest

Link to runnable example

No response

Steps to reproduce

browserify/node-util#62 (comment)

Expected behavior

Use console.assert instead.

Actual behavior

assert is a node built-in module.

Runtime

Node v14

Package manager

yarn v1

OS

macOS

Build and bundle tools

Vite

[JounQin]

util package is used by assert package, which is a built-in node module, but some packages can also be run on browser like micromark use assert inside, so I have to install npm assert on development and strip them on production building.

For now, I have to define a process.env.NODE_DEBUG in vite.config.ts for workaround.

[JounQin]

import type { ComponentType, FC, ReactElement } from 'react'
import { Fragment, useState } from 'react'
import rehypeDomParse from 'rehype-dom-parse'
import rehypeReact from 'rehype-react'
import remarkParse from 'remark-parse'
import remarkRehype from 'remark-rehype'
import { unified } from 'unified'

import { Loading } from './Loading'

import { useConstant, useEnhancedEffect } from 'hooks'

export const Rehype: FC<{ children?: string; markdown?: boolean }> = ({
  children,
  markdown,
}) => {
  const processor = useConstant(() => {
    let processor = unified()

    processor = markdown
      ? processor.use(remarkParse).use(remarkRehype)
      : processor.use(rehypeDomParse)

    return processor
      .use(rehypeReact, {
        createElement<T>(
          Element: ComponentType<T>,
          props: T,
          ...children: unknown[]
        ) {
          return <Element {...props}>{children}</Element>
        },
        Fragment,
      })
      .freeze()
  }, [markdown])

  const [result, setResult] = useState<ReactElement | null>()

  useEnhancedEffect(async () => {
    if (children == null) {
      setResult(null)
    } else {
      const { result } = await processor.process(children)
      setResult(result)
    }
  }, [children, setResult])

  return result === undefined ? <Loading /> : result
}

[wooorm]

See also: remarkjs/react-markdown#632 (comment)

---

Where did you quote #87 (comment) from? got it!

[JounQin]

@wooorm For browser/bundler friendly, there are two ways.

1. always use if (process.env.NODE_ENV != 'production') before assert or debug statement.
2. custom noop functions for them and add browser mapping in package.json

// assume.js
export const assertEqual = assert.equal
export const assertDeepEqual = assert.deepEqual
export const debug = _debug('micromark')

// assume-browser.js
export const assertEqual = noop
export const assertDeepEqual = noop
export const debug = noop

// package.json
{
  "exports": {
    "./assume.js": {
      "browser": "./assume-browser.js"
    }
  }
}

The first approach is a bit verbose but much more minify friendly for frontend project.

[ChristianMurphy]

I like the idea of being more browser compatible. 👍

always use if (process.env.NODE_ENV) != 'production' before assert or debug statement

I'm not sure if this would get us further towards being browser compatible?
process is node specific, process.env.NODE_ENV even more so.

custom noop functions for them and add browser mapping in package.json

This could work 🤔
vfile has used a similar approach for some time, patching in browser compatible noops/polyfills
https://github.com/vfile/vfile/blob/b4c9fb85e975be2b32e03e47c4bc8a1501155493/package.json#L38-L47

[JounQin]

I'm not sure if this would get us further towards being browser compatible?
process is node specific, process.env.NODE_ENV even more so.

process.env.NODE_ENV is bundler friendly.

For native browser loading via esm, if (typeof process !== 'undefined' && process.env.NODE_ENV != 'production') is required.

[jalik]

The rule when creating a lib, is to not make it dependent to any tools (bundler in this case), unless you are coding for yourself.
Using process or any nodejs related code is fine if the lib is meant to be used in that environment (node), otherwise it must be avoided.
For micromark, it's not clear if the lib was made for browser, nodejs or both.
Why micromark does not add "assert" as a dev dependency, so it's installed automatically?
Then you would just have to use if (typeof process !== 'undefined') where needed to fix the issues related to process.

[wooorm]

Re @JounQin:

1. always use if (process.env.NODE_ENV != 'production') before assert or debug statement.

Please look at how micromark works, here’s a small example:

micromark/packages/micromark-util-normalize-identifier/package.json

Lines 34 to 42 in b866e34

	"files": [
	"dev/",
	"index.d.ts",
	"index.js"
	],
	"exports": {
	"development": "./dev/index.js",
	"default": "./index.js"
	},

It’s using a very new, Node.js-specific, compile time, replacement of that. Defaulting to production code for everyone, and only when asked to load development code with this new feature, loads code with assertions/debug statements.

assert and debug are not available in the default production build. They only exist in development.

2. custom noop functions for them and add browser mapping in package.json

You can decide if you want that. You implicitly / your bundler explicitly ask micromark to load development code which with assertions.
If you don’t want the assertions, you can:

• Create an issue with vite so that it doesn’t explicitly pass --conditions development by default
• Create an issue with vite so that they allow assert (and maybe other builtins) in development
• Configure vite to not load development code explicitly
• Configure vite to map assert to a local package in development
• Configure vite to map assert to an empty packcage in development

---

Re: @jalik:

For micromark, it's not clear if the lib was made for browser, nodejs or both.

Both.

Why micromark does not add "assert" as a dev dependency, so it's installed automatically?

Node builtins should not be added to packages. They are not used. Some bundlers replace things with those dependencies though, but that’s left up to the final user (you).

---

Three ideas for micromark:

• @JounQin’s suggestion on console.assert — unfortunately console.assert misses features, such as:

  1. It prints a warning instead of throwing an error

  2. It’s not typed in TypeScript (because it doesn’t throw an error):

     function assert(value: unknown, message?: string | Error): asserts value;

     interface Console {
       assert(condition?: boolean, ...data: any[]): void;
       // ...
     }

• We could use power-assert, which is also supported by unassert
  1. has a @types/power-assert but without assertions:

     declare function assert(value: any, message?: string): void;

• We could use nested conditions:

   "exports": {
     "node": {
       "development": "./dev/index.js"
     },
     "default": "./index.js"
   },

  The downside is that it’s impossible for folks to get the development build outside of Node

[wooorm]

Btw, I honestly think this is something that Vite should fix:

Node does not use "development" by default, it has to be explicitly passed: node --conditions=development example.js.

I think Vite should behave the same

[JounQin]

@wooorm

Can we have

 "exports": {
   "development": "./dev/index.js",
   "default": "./index.js",
   "browser": {
     "development": "./index.js"
   }
 }

Will this work for all cases?

[wooorm]

Within the "exports" object, key order is significant. During condition matching, earlier entries have higher priority and take precedence over later entries. The general rule is that conditions should be from most specific to least specific in object order.

[...]

When using environment branches, always include a "default" condition where possible. Providing a "default" condition ensures that any unknown JS environments are able to use this universal implementation, which helps avoid these JS environments from having to pretend to be existing environments in order to support packages with conditional exports. For this reason, using "node" and "default" condition branches is usually preferable to using "node" and "browser" condition branches.
— https://nodejs.org/api/packages.html#packages_conditional_exports

Also, were you able to discuss this with Vite maintainers? I remain of the opinion that they should not default to an explicit --conditions=development

[JounQin]

OK, I just open vitejs/vite#4815 for tracking, it would be great if you could get involved.

[JounQin]

Besides, I'm not quite sure to understand your quote.

I tried the following and it's working:

 "exports": {
   "browser": "./index.js", // browser will always point to `./index.js` as entry
   "development": "./dev/index.js",
   "default": "./index.js"
 }

Please point me if I'm wrong.

[JounQin]

That's all package.json related

[wooorm]

My last comment had to do with the order. Your order earlier was different. Your order is okay now.

---

Your new example is similar to my last suggestion before: #87 (comment).
Except that your suggestion has an explicit browser field, and my suggestion has an explicit node field.
If we’d do this, I think I prefer an explicit Node field over an explicit browser field

But I think browsers should also be able to load development code.

[JounQin]

But I think browsers should also be able to load development code.

I don't get your point why browsers should load codes which are node specific module: assert.

[wooorm]

Browsers or bundlers?

Most bundlers handle this fine because they don’t load code marked explicitly as development code.

All bundlers can handle assert either by default or if end-users configure them.

[dctalbot]

Cross-referencing same problem in webpack land: remarkjs/remark#831

I'm not familiar enough with packaging to provide any real useful input, but I'm not excited at the prospect of more bundle config. Current best solution for me I think is manually installing assert as a sub-dependency

[maclockard]

If micromark is supposed to be environment agnostic, it should not rely on environment specific builtins, in this case, the node built-in assert. Browser's don't have assert built-in, so any code using it will break without a polyfill, regardless of the bundler.

If you expect users to polyfill this themselves, you should document that it is a required polyfill so folks expect this and properly configure their bundlers to handle node specific code.

Most bundlers handle this fine because they don’t load code marked explicitly as development code.

This isn't true, webpack loads code explicitly marked as development code when doing a non-production build/watch. I don't think its reasonable to expect all webpack consumers of this package to do a production build/watch whenever locally deving.

[wooorm]

@maclockard This thread needs a solution rather than convincing. I laid out three solutions at the end of this comment above, but they all have some problems. If you have ideas, or can spend the time working on overcoming those problems, that’d be appreciated.

webpack loads code explicitly marked as development code when doing a non-production build/watch.

It did that based on a NODE_ENV environment variable, not the Node-specific export map.

[ChristianMurphy]

@wooorm re:

We could use power-assert, which is also supported by unassert
has a @types/power-assert but without assertions

If definitely typed were updated to include type assertions in the power-assert typings.
Would there be any other concerns/blockers to adopting power-assert as a cross platform solution?

[wooorm]

Indeed, I think that’s the blocker.
I’ve never used it myself, so perhaps it doesn’t work well, but it says it has the same api as assert, so I think it would work!

[ChristianMurphy]

Upstream PR with fixes for power-assert typings at DefinitelyTyped/DefinitelyTyped#56062

[maclockard]

Sorry, misread some of the back-and-forth.

An alternative solution that I would be more than happy to contribute is just adding a utility function to this package with the same behavior as assert.

The advantages here would be fewer dependencies with more control over behavior. Disadvantages being its more code to maintain for this package.

[wooorm]

[edit: hit send too early]:

Thanks!

The advantages here would be fewer dependencies with more control over behavior.

micromark has a quite modern set up, which was Node specific, to load some extra code when explicitly debugging.
To summarize, the extra code is compiled away, you won’t see it in production, so using assert or powerassert won’t impact performance or bundle size.
But this problem arose because several tools now started defaulting to this Node-specific debugging code by default.

[ChristianMurphy]

Indeed, I think that’s the blocker.

Upstream PR with fixes for power-assert typings at DefinitelyTyped/DefinitelyTyped#56062

Fixes published in version 1.5.5 of the types https://www.npmjs.com/package/@types/power-assert/v/1.5.5

[wooorm]

Thanks @ChristianMurphy for updating the types. I’ve released all micromark extensions to not include node:assert in their development builds, but to instead use power-assert.