browser breakage in mobx@6 esm: "process is not defined"

chase-moskal commented 4 years ago

Intended outcome:
i tried to upgrade from mobx@5 to mobx@6 in my esm app

Actual outcome:
browser throws error in mobx.esm.js upon encountering reference to node-specific process.env

How to reproduce the issue:

in this codepen example, open your browser's js console, run the codepen, and you'll see the error process is not defined

now switch to mobx@5 by replacing the import map:

{
  "imports": {
    "mobx/": "https://unpkg.com/mobx@5.15.7/",
    "mobx": "https://unpkg.com/mobx@5.15.7/lib/mobx.module.js"
  }
}

run again and you'll see the problem is gone in v5, and the mobx module is logged to console

My thoughts:

the problem appears when i move from @5.15.7 mobx.module.js to @6.0.1 mobx.esm.js
i see process.env in both of those files.. it looks like v5 does some magic to write a fake process object when it's missing globally
i think it would be preferable if mobx didn't write to the global process object at all. sometimes, other programs might check the existence of that object to detect node environment, so if the solution is like mobx@5 here, mobx@6 could accidentally interfere with another system's environment detection, causing tricky bugs

mweststrate commented 4 years ago

If you want to consume MobX directly in the browser, I recommend to use the .min.js or .umd.js builds instead. I still don't know what the expected behavior of libraries is here in the current JS eco system, as bundlers use process.env.NODE_ENV (or some arbiratrily other global) to optimize their builds to conditionally include / exclude debug information and such.

On Mon, Oct 26, 2020 at 9:52 PM Chase Moskal notifications@github.com wrote:

Intended outcome: i tried to upgrade from mobx@5 to mobx@6 in my esm app

Actual outcome: browser throws error in mobx.esm.js https://unpkg.com/mobx@6.0.1/dist/mobx.esm.js upon encountering reference to node-specific process.env

How to reproduce the issue:

in this codepen example https://codepen.io/ChaseMoskal/pen/jOrBXjB?editors=1001, open your browser's js console, run the codepen, and you'll see the error process is not defined

now switch to mobx@5 by replacing the import map:

{ "imports": { "mobx/": "https://unpkg.com/mobx@5.15.7/", "mobx": "https://unpkg.com/mobx@5.15.7/lib/mobx.module.js" }}

run again and you'll see the problem is gone in v5, and the mobx module is logged to console

My thoughts:

the problem appears when i move from @5.15.7 mobx.module.js https://unpkg.com/mobx@5.15.7/lib/mobx.module.js to @6.0.1 mobx.esm.js https://unpkg.com/mobx@6.0.1/dist/mobx.esm.js

i see process.env in both of those files.. it looks like v5 does some magic to write a fake process object when it's missing globally

i think it would be preferable if mobx didn't write to the global process object at all. sometimes, other programs might check the existence of that object to detect node environment, so if the solution is like mobx@5 here, mobx@6 could accidentally interfere with another system's environment detection, causing tricky bugs

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx/issues/2564, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4NBDCBXC4O6IPCWDPJ7LSMXVTVANCNFSM4S764IMA .

chase-moskal commented 4 years ago

@mweststrate

okay hold on — i'm certain i accidentally freaked you out in my example with usage of community tooling like es-module-shims... it probably looks to you like i'm doing something hacky or experimental and deserve to be left unsupported... however this is a mistake: please, let me reframe the issue, stripping away the community-tooling, to highlight the issue against ratified standards, as i should have from the start

all modern browsers support es modules
mobx@6 publishes a module which fails to load in web browsers.
users should expect this to work, but it fails
```
<script type="module">
  import * as mobx from "https://unpkg.com/mobx@6.0.1/dist/mobx.esm.js"
</script>
```

this issue is a regression introduced in mobx@6
as we can see, mobx@5 works correctly in this context

<script type="module">
  import * as mobx from "https://unpkg.com/mobx@5/lib/mobx.module.js"
</script>

we just want the module to work in web browsers the standard way, as of course it should.
personally, all my work has been strictly esm-only since the start of the year, and i love it!

anyways, this issue is actually a very minor defect with a simple fix, without drawbacks or difficulty.
i propose that mobx starts loading with a step that passively gathers information about the environment. maybe something along these lines:

const mobxEnv: {node: boolean; debug: boolean} = (
  typeof process === "undefined"
    ? {
      node: false,
      debug: false,
    }
    : {
      node: !!process.version,
      debug: process.env.NODE_ENV !== "production",
    }
)

of course, if mobx architecture allows for it, it would be best if details like these could be explicitly overwritten for each manually-instantiated mobx context

then of course, mobx uses the environment info internally to make decisions during its operation

die(mobxEnv.debug ? "message for debug" : "message for production")

such a fix would

make mobx importable the standard way, benefitting noobs and experts alike
have no breaking changes for anybody
avoid writing to the process global, so we can avoid interfering with other programs

we can easily make mobx standards-compliant and work smoothly for everybody — i'm certain of it!

would you consider merging a pull request for this fix?

:wave: chase

danielkcz commented 4 years ago

Well, this is rather difficult. Honestly, I see this for the first time to load ESM in a browser like that. I would consider that an edge case. It's not widely adopted for sure.

Your "fix" would help in your case, but it would mean that regular bundlers cannot drop unused (dev) code. Your solution is too runtime to be statically analyzable.

You might notice we are using __DEV__ constant in many places of the code which is translated by tsdx tool during the build into process.env.NODE_ENV === "development" so the rollup can see that and produce a correct output.

urugator commented 4 years ago

I think that defaulting process.env to an empty object (as in Mobx5) should be quite safe and it's convinient even with bundler - no need to configure "replace" to get started. Or is it somehow problematic?

chase-moskal commented 4 years ago

@FredyC

okay, i understand. so mobx has special tooling-specific instructions in the module that breaks in web browsers. in the current case, mobx supports bundlers, not browsers

but keep this in mind: this issue is a recent regression in mobx@6, as mobx@5 works fine. so i've downgraded my apps to mobx@5 in the meantime

Honestly, I see this for the first time to load ESM in a browser like that. I would consider that an edge case. It's not widely adopted for sure.

hah, i know it seems unusual. i mean, we're all experts who are so familiar with our powerful tooling like rollup, npm, and the rest.. now that we have these mastered, why imagine doing anything differently? well, as usual, things are changing on the web..

i work with cutting edge practices so that i can be a canary the coal mine. younger developers will be finding the new official recommended techniques from MDN and WHATWG, and it looks like this

<script type=module src="mobx6-is-broken.js"></script>

it's no hipster edge-case. it's just how javascript works now and moving forward.
since it's so simple and lean, it's how many developers will be taught

when a young developer sees that mobx fails to load, and the official explanation from the mobx team is: "no no silly, mobx doesn't 'just work' as a javascript module... first, you have to install node, and npm, and establish a build toolchain starting with a rollup config, and don't forget the node-resolver, and and and"... this just can't be where we land

fixing this issue for mobx will prevent many more devs from making the same confused groaning noises as me yesterday when upgrading mobx broke my app ;)

possible solution: add module mobx.universal.js

environment agnostic (works everywhere, browsers, node, and deno)
perhaps suffers of bloat? (bundler couldn't optimize out naughty bits, right?)

here's a different solution

mobx.module.js — no change, has the bundler-specific instructions for optimizations
mobx.browser.js — new module is optimized for browsers (should work for deno too)
mobx.node.js — new module is optimized for node

i suppose one or both of those solutions could be employed. however it's perhaps worth a deeper reconsideration of the general way mobx is optimizes itself for target environments

another way to look at this, is that mobx currently only supports bundlers (and, i guess, node): but i'm asking for mobx to at least add browser support, or even ideally, offer universally-agnostic support for all javascript environments

let's make mobx work in browsers. we can do it!

:wave: chase

danielkcz commented 4 years ago

I can't really imagine what is the benefit of loading ESM in the browser. Generally, the ESM is useful as it's statically analyzable and can be trimmed down only to things you have referenced in other modules. However, in the browser, you have to fetch the whole thing from the network anyway, so it seems to be losing that benefit?

That said, if you would use the UMD bundle we have, what would you lose there? http://unpkg.com/mobx

And I wonder if you can use TypeScript in your code when loading modules this way? It feels like it's too detached from the actual code. Hard to imagine that this would be a way forward if something like that would be missing.

mweststrate commented 4 years ago

Browser ESM will be a big thing I expect soon. A big benefit is that it makes the development cycle much more efficient as it doesn't need bundling (I think snowpack does this already iirc) and it's interesting for edge caching as well. Feel free to open a PR to configure the build to generate the .browser version? Curious were we'd run into. Note that you presumably want a minified version as well.

Op di 27 okt. 2020 21:49 schreef Daniel K. notifications@github.com:

I can't really imagine what is the benefit of loading ESM in the browser. Generally, the ESM is useful as it's statically analyzable and can be trimmed down only to things you have referenced in other modules. However, in the browser, you have to fetch the whole thing from the network anyway, so it seems to be losing that benefit?

That said, if you would use the UMD bundle we have, what would you lose there?

And I wonder if you use TypeScript in your when loading modules this way? It feels like it's too detached from the actual code.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx/issues/2564#issuecomment-717560426, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4NBGRC3SHC2I7NJ4V2FLSM455XANCNFSM4S764IMA .

urugator commented 4 years ago

the official explanation from the mobx team is

The official explanation is that you have to somehow tell the library in which mode to run. We follow widely adopted convention - set process.env.NODE_ENV to either development or production. process.env.NODE_ENV may seem wierd in non-node context, but that's exactly the point - use the same convention for all envs (it's process.env.NODE_ENV more or less for historical reasons). It doesn't mean the build is optimized for node and it doesn't matter which tooling you use - if you don't provide process.env.NODE_ENV it will break (even with bundler). You can eg import "./setup-env.js" before other imports:

// setup-env.js
globalThis.process = { env:  { NODE_ENV: 'development' } }

We do provide best efford, as agnostic as possible, production build - the mentioned min.js. It's not ESM, because that would assume an env that supports ESM.

What you're asking for is opinionated production and development build that targets latest ES.

EDIT: Based on personal experience not using compiler/bundler for production is non-realistic atm (ES/code elimination/testing/JSX/HTTP2/QUIC...) - haven't tried any 3rd party services. For prototyping/development the biggest problem really is the support from libraries. The umd builds are quite usable, but not ideal - one have to either reexport the API or use globals. However once I setup bundling for production, it becomes inconvinient to use different workflow/techniques for development, it's a vicious circle. Rather than .browser I think it would make more sense to have something like: mobx.esm.development.js mobx.esm.production.js mobx.esm.production.min.js

danielkcz commented 4 years ago

I think this discussion would be better to move to tsdx and solve it there. If this approach is truly "the future", the MobX won't be the only one that needs handling it properly.

@chase-manning Do you mind opening an issue with them? Ideally, if you could also contribute PR. They don't have many active maintainers lately, but we can always fork if it comes to big delays.

Fun fact, a year ago I championed to have a single ESM output with process.env untouched: https://github.com/formium/tsdx/issues/140

@mweststrate

Browser ESM will be a big thing I expect soon. A big benefit is that it makes the development cycle much more efficient as it doesn't need bundling

I get that, but I am trying to figure out what's different from using current UMD vs browser compatible ESM.

danielkcz commented 4 years ago

Wait, there was already an attempt to handle it on tsdx side: https://github.com/formium/tsdx/issues/631

Interestingly enough, this has to lead me to immer which suffers from the same problem and there is a suggestion with tsdx that's worth the try: https://github.com/immerjs/immer/issues/557#issuecomment-602647766

Anyway, I have a feeling this whole thing brings yet another annoying fork into the ecosystem. It would be brilliant if the browser could handle TypeScript too, because then we wouldn't need to do anything, just publish source files and let the browser pick what it needs. Also what about SSR 🤯

chase-moskal commented 4 years ago

@FredyC

I get that, but I am trying to figure out what's different from using current UMD vs browser compatible ESM.

Anyway, I have a feeling this whole thing brings yet another annoying fork into the ecosystem.

i'm worried i've confused you, so let me try to be less verbose here

i'm really just asking for mobx to support web browsers, the officially standardized way, via script tag, just like any other normal es module in the modern ecosystem

<script type=module>

  // MOBX FAILS: process is not defined
  import * as mobx from "https://unpkg.com/mobx@6.0.1/dist/mobx.esm.js"

  // but other modules just work
  import * as lithtml from "https://unpkg.com/lit-html@1.3.0/lit-html.js"

</script>

this technique is now how many junior developers are being taught to load dependencies. our familiar old bundling tooling is no longer required to load dependencies, thus making optional the tooling and advanced optimizations which interest devs like us

upgrading from mobx 5 to 6 broke my app, because for the last year, my apps have been loading dependencies standard way like above to achieve an awesome developer experience

i'm only asking for mobx to add this basic browser support. i'm not asking for anything existing to be removed or even changed

tsdx

i'd love to, but i just now cannot afford the time to sufficiently investigate or cross-reference the workings of mobx and tsdx

@urugator

okay. when looking at mobx/dist/ i now see what you're saying: let's see if we're on the same page about this

for umd we have prod/min/dev

mobx.umd.production.js
mobx.umd.production.min.js
mobx.umd.development.js

for cjs we have prod/min/dev

mobx.cjs.production.js
mobx.cjs.production.min.js
mobx.cjs.development.js

but for esm we have something different

mobx.esm.js

and the proposed fix, is to publish similar prod/min/dev bundles for esm

mobx.esm.production.js
mobx.esm.production.min.js
mobx.esm.development.js

being pre-optimized, those new esm bundles won't have any of the node-specific instructions, and thus should be browser-safe. i think this approach should fix this issue outright, leaving no breaking changes, making everybody happy :)

thoughts?

:wave: chase

PS — i can't afford to write a PR now, but i'll try to be available for discussion and review. cheers!

urugator commented 4 years ago

and the proposed fix, is to publish similar prod/min/dev bundles for esm

I sort of agree, but there is a slight difference ... these cjs/umd builds target ES5, they're already transpiled as they favor maximum compatibility ... it probably doesn't make sense to do the same for esm build, since ESM is already non ES5 feature. So the question is which ES version or rather which browser or node/deno version should we target with these builds? Or should we only assume support for ESM, but nothing else? EDIT: probably yes, current esm build does that as well...

won't have any of the node-specific instructions, and thus should be browser-safe

Again. All current builds are browser safe, there are no node-specific instructions. Just the esm.js build requires you to set a variable, that is by convention named process.env.NODE_ENV. It could be equally well named mobxEnv, but then we would force everyone to customize their build process specifically for mobx. The other builds don't have to specify this variable, because they're pre-build with this variable replaced by constant value, which allows to remove devel related code and therefore save size and preformance. On the other hand you loose the ability to set this variable dynamically with these builds - you have to change the imports. What I agree about is that it shouldn't crash, but fallback to development (as in Mobx5) and perhaps console.log("Mobx runs in development mode, for production mode please set process.env.NODE_ENV to "production")

EDIT: I think we don't have to provide non-minified production version, unless there would be a demand for it.

EDIT2: Yea I think adding mobx.esm.production.min.js, mobx.esm.development.js using the same transpilation rules would make sense.

danielkcz commented 4 years ago

@urugator I am going to correct you a bit. The ESM build is actually very similar to CJS except the export/import syntax is not transpiled away. Both of those bundles are targeting ES5 syntax for compatibility. And the other difference as you correctly said the process.env.NODE_ENV is left in ESM on purpose because there was a general assumption that people consuming ESM have to use some bundler anyway which would deal with it. That's obviously changing now, but it's not that simple...

There is one important reason why it's currently like this you are both missing. The module field in package.json is the one used by the bundler to decide which file to use. It works well with a single ESM that has process.env.NODE_ENV still present and the bundler can inject a correct value. At the moment we separate ESM bundle, we would naturally have to use module=mobx.esm.production.min.js so the final result is bundled with production optimizations. But people won't be able to use development one without some ugly aliasing hacks.

What I agree about is that it shouldn't crash, but fallback to development (as in Mobx5) and perhaps console.log("Mobx runs in development mode, for production mode please set process.env.NODE_ENV to "production")

I do agree with this. It's a viable workaround without messing it up for others. Until this new & hot approach is more widespread, let's go this way.

@chase-moskal Note that whenever you would want to use another package that's bundled with TSDX (eg. Formik, Immer, ...) you would be dealing with the same issue. Instead of going to the maintainers of every package, you should go after the source of the problem. If you don't want to escalate that, just set that variable and be happy :)

danielkcz commented 4 years ago

Btw, another important detail that's not so obvious. With the CJS, there is a simple index.js that looks like this. That way we can have main field in package.json and the correct file is consumed based on environment.

if (process.env.NODE_ENV === 'production') {
  module.exports = require('./mobx.cjs.production.min.js')
} else {
  module.exports = require('./mobx.cjs.development.js')
}

However, we can't have the same thing for ESM ... because ESM cannot import conditionally and both versions would be bundled. If you would use CJS to load ESM, you are losing all benefits of it.

jeremy-coleman commented 4 years ago

Fun fact, because esm is a live binding v8 can share memory across v8 isolates for esm in the same way it does for all the built in objects. Its kind of a big deal, if you have a lot of iframes or embeds or 2 tabs on the same realm. Just make a es5 target with esm syntax , its such an easy fix.

mweststrate commented 4 years ago

@jeremy-coleman please mind your tone.

We're serving many users here and have to maintain stuff we change for a long time, so we try to move intentionally. If you are looking for an easy fix and have no patience to await a diligent process, you can just add something like the following to your index and you're done : <script>global.process = { env: undefined }</script>

I'm curious how you are loading react, because they don't offer an esm build either.

danielkcz commented 4 years ago

@chase-moskal Can you please elaborate on how the ESM browser imports work in development? I mean the type="module" can specify either production.min or development. How is the workflow in that case? Do you manually switch imports when pushing to production? Or there is no development support? Hard to believe it would be "next best thing" if that would be the case.

That said, would it be enough if we add a production bundle that has NODE_ENV correctly stripped down? Do you have some opportunity to actually use the development bundle?

jeremy-coleman commented 4 years ago

Sorry for wrong tone, just writing style no ill feelings. I dont load using cdn like chase is trying to, i rebundle deps as esm using rollup and exclude all eternals in application code (to reap the benefit of shared v8 heap). I was just making the point there is a huge runtime benefit (that few people know about) for practically 0 effort of publishing a minified esm bundle for cdns, although i personally have no use for it, a lot of people do.

V8 can optimize esm imports in the same way the builtins are optimized as discussed here. https://v8.dev/blog/embedded-builtins

chase-moskal commented 4 years ago

@FredyC — i'll try my best to explain

here's my simplified scenario, let me know if this makes sense

my browser app consumes es modules
my app is using import maps to instruct the browser where to find packages
- thus, i can configure an alias to point mobx at the development bundle whenever necessary
my app has a development build and a production build
- in production, my app would assume the default module entrypoint for mobx would naturally point at mobx.esm.production.js
  - the package.json module field would need to be set this way
- in development, my app would alias mobx to point at mobx.esm.development.js

so from my perspective, the procedure to fix this issue might look something like

add mobx.esm.production.js and mobx.esm.development.js to dist
set package.json module field to mobx.esm.production.js (maybe setting jsnext:main and react-native too?)

also, i have an idea about the process object shenanigans

as you know, i'm concerned about mobx having a global side effect like this: we're dressing up the window object in a node-looking dress so we can fool bundler conventions into thinking we're a particular node environment — what's scary about that, is that we might get more than we bargain for, and start fooling other javascript programs on the page, which may then malfunction in tricky ways
it may eventually be worth considering a serious refactor in this area, may or may not involve tsdx
however, at the very least, we can at least strive to clean up the mess we make! if we must write a process object, let's do it only while mobx is loading — then afterwards, we set the global back to whatever it was before we started loading mobx — this way, we can save the day by using the process object hack, and we can be polite about it — win win!

@mweststrate

I'm curious how you are loading react, because they don't offer an esm build either.

since i've moved on from react last year, i've been having a stellar time writing 100% esm apps and true web components with lit-element and mobx with the help of @adobe/lit-mobx

i'd say lit-element is an excellent example of a well-formed esm library in the modern ecosystem worth emulating. they've achieved a marvelous minimalism and set the bar higher than most. very interestingly, they refuse to distribute any bundles, or minifications, or environment-specifics in a library, identifying those as footguns bad for consumers... so instead, lit-element only distributes simple pristine es modules... in the longer term, i'd really like to see mobx@7 take inspiration from these simpler practices, go esm-first, cut away some obsolete complexity, and join as a proud leader of our newly emerging javascript ecosystem... food for thought! :)

anyways, back to the primary issue, i think we'll generally agree on this solution:

write the process object (then clean it up afterwards!) in the two new esm bundles so browsers don't crash
and set package.json 'module' field to the production bundle

:wave: chase

danielkcz commented 4 years ago

add mobx.esm.production.js and mobx.esm.development.js to dist set package.json module field to mobx.esm.production.js (maybe setting jsnext:main and react-native too?)

We can do the first thing, but the second thing would break it for bundlers as I explained in https://github.com/mobxjs/mobx/issues/2564#issuecomment-718242870. You will have to set mapping for both cases unfortunately until someone comes with standardized package fields for this use case.

it may eventually be worth considering a serious refactor in this area, may or may not involve tsdx

Well, considering you the first one to be concerned by this compared to thousands of people going with "old way", I don't think that will happen anytime soon ;)

Thanks for the writeup anyway. It still sounds like a horrible developer experience to me, but it's fine if you are satisfied. I've already included this in the checklist for the upcoming monorepo (#2530) where we will unify this across packages. I hope you can cope with V5 for now :) Closing and please follow the linked issue.

danielkcz commented 3 years ago

@chase-moskal The mobx 6.0.3 now includes ESM bundles without NODE_ENV. It was somewhat painful to convince tsdx about it, but I've managed.

data-handler commented 3 years ago

I'm very confused (and stupid)

I'm using mobx 6.0.4 via npm install

I'm bundling via rollup

When I load my app in the browser I get:

mobx.esm.js:73 Uncaught ReferenceError: process is not defined

mobx source: var errors = process.env.NODE_ENV !== "production" ? niceErrors : {};

I know it's difficult to provide an answer when you can't see my set up, but I thought this particular issue was fixed in mobx 6.0.3 as per @FredyC's comment?

urugator commented 3 years ago

You have to import either: mobx.esm.production.min.js for production mobx.esm.development.js for development

danielkcz commented 3 years ago

It's kinda surprising why rollup does not have process available since it's effectively a node process. Seems like something else is smelly in your setup. It shouldn't be necessary to pick a specific bundle in a rollup environment.

data-handler commented 3 years ago

Okay. Thank you @urugator and @FredyC

urugator commented 3 years ago

Or try rollup.config.js:

import resolve from '@rollup/plugin-node-resolve';
import babel from '@rollup/plugin-babel';
import commonjs from '@rollup/plugin-commonjs';
import replace from '@rollup/plugin-replace';
import { terser } from 'rollup-plugin-terser';

export default [
  {
    input: 'public/index.js',
    output: {
      file: 'public/index.development.js',
      format: 'iife',
      name: 'myproject',
      plugins: [],
    },
    plugins: [
      replace({
        'process.env.NODE_ENV': JSON.stringify('development'),
      }),
      resolve({ browser: true }),
      commonjs(),
    ],
  },
  {
    input: 'public/index.js',
    output: {
      file: 'public/index.production.js',
      format: 'iife',
      name: 'myproject',
      plugins: [
        terser(),
      ],
    },
    plugins: [
      replace({
        'process.env.NODE_ENV': JSON.stringify('production'),
      }),
      resolve({ browser: true }),
      commonjs(),
      babel({ babelHelpers: 'bundled' }),
    ],
  },
];

EDIT: you have to npm install the imported plugins

data-handler commented 3 years ago

@urugator This worked for me. Thanks again

mobxjs / mobx

browser breakage in mobx@6 esm: "process is not defined" #2564

That said, if you would use the UMD bundle we have, what would you lose there?