Remove support for `NODE_ENV`

remcohaszing commented 1 year ago

Initial checklist

[X] I read the support docs
[X] I read the contributing guide
[X] I agree to follow the code of conduct
[X] 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. :shrug:

wooorm commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

What? Noo, I mean:

development = options.development

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

remcohaszing commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

remcohaszing commented 1 year ago

Node.js has conditions.

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

I meant this to apply to @mdx-js/node-loader, as you implemented. I was also thinking of @mdx-js/register, although I also wouldn’t mind deprecation of that one.

Just for clarification, I expect the following to be a built tool program that creates a development build. For example, the process might use a production micromark package, but the output would contain a development version of the micromark package.

some-build-tool --mode development

I expect the following to create a production build, but provide more verbose information and checks in the build tool process. For example, this process might output verbose logging coming from micromark, but the output would contain an optimized production version.

node -c development node_modules/.bin/some-build-tool --mode production

In case of a Node loader, the Node.js process is both the build tool and the runtime. Because it’s the runtime, I expect it to toggle development option based on the development condition.

I can implement development mode for Vite. It doesn’t involve having Vite as a dependency, only as a dev dependency for testing.

wooorm commented 1 year ago

@mdx-js/register

That project evaluates. So you have to pass it a runtime. Meaning you can just as well pass a dev runtime and development: true. Would just likely create the same confusion we had earlier

vite

Fine!

mdx-js / mdx