`assert` is not browser friendly about micromark HOT 28 CLOSED

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.

from micromark.

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:

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

from micromark.

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.

from micromark.

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.

from micromark.

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!

from micromark.

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

from micromark.

[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.

from micromark.

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.

from micromark.

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
}

from micromark.

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

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

from micromark.

@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.

from micromark.

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

from micromark.

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.

from micromark.

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.

from micromark.

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

from micromark.

@wooorm

Can we have

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

Will this work for all cases?

from micromark.

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

from micromark.

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

from micromark.

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.

from micromark.

That's all package.json related

from micromark.

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.

from micromark.

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.

from micromark.

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

from micromark.

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.

from micromark.

@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.

from micromark.

@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?

from micromark.

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

from micromark.

Hi! This was closed. Team: If this was fixed, please add phase/solved. Otherwise, please add one of the no/* labels.

from micromark.