browser breakage in mobx@6 esm: "process is not defined" #2564
Comments
|
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 ***@***.***> wrote:
*Intended outcome:*
i tried to upgrade from ***@***.*** to ***@***.*** in my esm app
*Actual outcome:*
browser throws error in mobx.esm.js
***@***.***/dist/mobx.esm.js> upon encountering
reference to node-specific process.env
*How to reproduce the issue:*
1. 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
2. now switch to ***@***.*** by replacing the import map:
{
"imports": {
"mobx/": ***@***.***/",
"mobx": ***@***.***/lib/mobx.module.js"
}}
3. 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
***@***.***/lib/mobx.module.js> to @6.0.1
mobx.esm.js ***@***.***/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 ***@***.***
here, ***@***.*** 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
<#2564>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAN4NBDCBXC4O6IPCWDPJ7LSMXVTVANCNFSM4S764IMA>
.
|
|
okay hold on — i'm certain i accidentally freaked you out in my example with usage of community tooling like
we just want the module to work in web browsers the standard way, as of course it should. anyways, this issue is actually a very minor defect with a simple fix, without drawbacks or difficulty. const mobxEnv: {node: boolean; debug: boolean} = (
typeof process === "undefined"
? {
node: false,
debug: false,
}
: {
node: !!process.version,
debug: process.env.NODE_ENV !== "production",
}
)
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
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? 👋 chase |
|
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 |
|
I think that defaulting |
|
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
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. 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
here's a different solution
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! 👋 chase |
|
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. |
|
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
<#2564 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAN4NBGRC3SHC2I7NJ4V2FLSM455XANCNFSM4S764IMA>
.
|
The official explanation is that you have to somehow tell the library in which mode to run. // setup-env.js
globalThis.process = { env: { NODE_ENV: 'development' } }We do provide best efford, as agnostic as possible, production build - the mentioned What you're asking for is opinionated production and development build that targets latest ES. EDIT: |
|
I think this discussion would be better to move to @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
I get that, but I am trying to figure out what's different from using current UMD vs browser compatible ESM. |
|
Wait, there was already an attempt to handle it on Interestingly enough, this has to lead me to 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 🤯 |
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
i'd love to, but i just now cannot afford the time to sufficiently investigate or cross-reference the workings of mobx and tsdx 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
for cjs we have prod/min/dev
but for esm we have something different
and the proposed fix, is to publish similar prod/min/dev bundles for esm
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? 👋 chase PS — i can't afford to write a PR now, but i'll try to be available for discussion and review. cheers! |
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
Again. All current builds are browser safe, there are no node-specific instructions. Just the 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 |
|
@urugator I am going to correct you a bit. The There is one important reason why it's currently like this you are both missing. The
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 :) |
|
Btw, another important detail that's not so obvious. With the CJS, there is a simple 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 |
|
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. |
|
@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 : I'm curious how you are loading react, because they don't offer an esm build either. |
|
@chase-moskal Can you please elaborate on how the ESM browser imports work in development? I mean the That said, would it be enough if we add a production bundle that has |
|
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. |
|
@FredyC — i'll try my best to explain here's my simplified scenario, let me know if this makes sense
so from my perspective, the procedure to fix this issue might look something like
also, i have an idea about the process object shenanigans
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:
👋 chase |
We can do the first thing, but the second thing would break it for bundlers as I explained in #2564 (comment). You will have to set mapping for both cases unfortunately until someone comes with standardized package fields for this use case.
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. |
|
@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. |
|
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? |
|
You have to import either: |
|
It's kinda surprising why |
|
Or try 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 |
|
@urugator This worked for me. Thanks again |
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.envHow to reproduce the issue:
process is not definedMy thoughts:
process.envin both of those files.. it looks like v5 does some magic to write a fakeprocessobject when it's missing globallyThe text was updated successfully, but these errors were encountered: