Remove support for `NODE_ENV`

[remcohaszing]

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)

Problem

I think the development option offers useful features, but I don’t think it’s good for a library to output different content based on an environment variable. I do think it’s useful for an integration to leverage a mode to determine what the output looks like.

• Webpack has mode. (This is already used)
• Vite has mode.
• Node.js has conditions.
• Rollup has no such option as far as I’m aware.
• ESBuild has no such option as far as I’m aware.

Especially when it comes to MDX on demand I believe it’s good to be explicit about this, because it determines whether the user should use /jsx-runtime or /jsx-dev-runtime.

Solution

• Default development to false, not process.env.NODE_ENV === 'development'
• @mdx-js/rollup: Support Vite env.mode
• @mdx-js/node-loader: Use the development exports condition in package.json to enable development mode.
• @mdx-js/register: Use the development exports condition in package.json to enable development mode.

This means Rollup, esbuild, and direct users of @mdx-js/mdx need to specify development: true explicitly.

Alternatives

Keep it as-is. 🤷

[wooorm]

Perhaps fine to drop NODE_ENV and only go with conditions, on the condition that we can solve the eval problem.
What if we do automatically do this when compiling to a program (common case in all the integrations and the lib by default), but not when function-body?

[remcohaszing]

Just for clarification, I think you mean the development option and runtime needs to match up between compile() and run() as per https://mdxjs.com/guides/mdx-on-demand, right?

Since we receive the runtime as an argument when compiling to a body, we could support both the JSX runtime and the JSX dev runtime if development: true. This means it’s always safe to pass the production runtime, but you won’t get the dev runtime benefits if you use that.

  /*@jsxRuntime automatic @jsxImportSource react*/
  const {
    Fragment: _Fragment,
-   jsxDEV: _jsxDEV
+   jsx: _jsx,
+   jsxs: _jsxs,
+   jsxDEV: _jsxDEV = (type, children, key, isStatic) => (isStatic ? _jsxs : _jsx)(type, children, key)
  } = arguments[0];
  function _createMdxContent(props) {
    // …
  }
  function MDXContent(props = {}) {
    // …
  }
  return {
    default: MDXContent
  };

[wooorm]

Yes, they need to match up.

My proposal is to infer development vs production, when we can import both, based on a development condition (that is, when compiling an entire program).

We could also always inject more code to “gracefully” switch. As you propose. But injecting extra code always may not be worth it. Particularly as people are passing one or the other runtime (either to evaluate or when “mdx on demand”). We can crash by default or enforce an explicit development: true if a jsxDEV is passed, too.

[remcohaszing]

Trying to wrap my head around how this would work.

compile would get a new boolean option, let’s call it devRuntime for now. This option determines whether the production or the dev runtime is used. It defaults to the same value as development.

import { compile } from '@mdx-js/mdx'

// Use production runtime
await compile('', {
  development: false,
  // implies
  // devRuntime: false
})

// Use development runtime
await compile('', {
  development: true,
  // implies
  // devRuntime: true
})

// Use development runtime
await compile('', {
  development: false,
  devRuntime: true
})

// Use production runtime
await compile('', {
  development: true,
  devRuntime: false
})

evaluate() does not accept this option. Instead, it requires either a production or a development runtime. Internally it will set { devRuntime: Boolean(jsxDEV) } when calling compile().

We could also always inject more code to “gracefully” switch. As you propose. But injecting extra code always may not be worth it.

I think this is well worth it for MDX on demand. It’s just a small snippet and it’s only injected in the development runtime. If we remove support for NODE_ENV, users might set it manually. It could come as a total surprise when code works in development, but breaks in production, or the other way around.

[wooorm]

What? Noo, I mean:

development = options.development

if output == program and typeof development != boolean
  development = inferDevelopmentFromCondition()

[remcohaszing]

Do you mean Node conditions? I feel the same about that as I do about environment variables.

but I don’t think it’s good for a library to output different content based on an environment variable.

I think it’s great to use this mechanism to produce verbose logging, or make additional assertions (either as warnings or errors), but I don’t think this should affect the output of a compiler.

[wooorm]

Yes.

You mentioned:

Node.js has conditions.

Hence I assumed you wanted this project, when conditions are used, to support conditions?

---

Why do you not want to use the development condition if it is explicitly turned on by a user and it can safely be used?

[wooorm]

I don’t think this should affect the output of a compiler.

Why not? As it enabled the phrase before: “I think it’s great to use this mechanism to produce verbose logging, or make additional assertions (either as warnings or errors)”

[wooorm]

ok went with what you proposed, as we run tests in development, it’s verbose to do this on the condition.

I didn’t investigate Vite. It would require pulling in Vite. Folks can use different configs in Vite anyway.