disable debug names in production builds

[iChenLei]

Code change checklist

• Added/updated unit tests
• Updated /docs. For new functionality, at least API.md should be updated
• Verified that there is no significant performance drop (npm run perf)

Updated /docs. For new functionality, at least API.md should be updated
👆 Need add it in official document ?

PR Background

reopen pr for #2748

Sumaary

Why not just add __DEV__ for getNextId()

this.keysAtom_ = createAtom(__DEV__ ? `${this.name_}.keys()` : "ObservableMap.keys()")  
 // 👆  e.g. why do this?

getNextId() is default debugnames gen util, and most time we will create new Atom, ObservableValue with name constructor params, so it also create many cancatenated string and string in JS Heap.(more details -> #2737

Run simple benchmark

import { observable } from 'mobx';

const HugeRootMobx = {};
for (let i  = 0; i < 100000; i++) {  👈🏻   // Yeah, One hundred thousand loop
 HugeRootMobx[i] = observable({ val: i });
}

window.mobxTest = HugeRootMobx;

Test on microsoft edge 87

This is JS Heap compare between different logic process.

If we only deal with getNextId(), not optimized enough .

@mweststrate Need your code review, maybe I missing something. Thank you, sir 😄

[changeset-bot]

🦋 Changeset detected

Latest commit: 9b00a94

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
mobx	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[mweststrate]

Thanks for looking into this. Looks awesome! Small request for changes as I think the mapid_ can probably be dropped entirely (or it is used somewhere, and in that case I'm not sure the change is safe)

[mweststrate]

Looking at this one, it seems that mapid_ is used nowhere. Feel free to drop it entirely if that still passes the test

[iChenLei]

If we delete mapid_ directly, The Unit Tests will be failed.

1. keep it
2. delete it and jest --updateSnapshot

Choice which one ? Sir.

[mweststrate]

Same (probably the IDerivation interface needs to adapted as well)

[mweststrate]

2 😊

…

On Sat, 6 Feb 2021, 14:23 ChenLei, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In packages/mobx/src/core/computedvalue.ts <#2780 (comment)>: > @@ -88,7 +88,7 @@ export class ComputedValue<T> implements IObservable, IComputedValue<T>, IDeriva lastAccessedBy_ = 0 lowestObserverState_ = IDerivationState_.UP_TO_DATE_ unboundDepsCount_ = 0 - mapid_ = "#" + getNextId() + mapid_ = __DEV__ ? "#" + getNextId() : "#" If we delete mapid_ directly, The Unit Tests will be failed. 1. keep it 2. delete it and jest --updateSnapshot Choice which one ? Sir. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2780 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBDM5ONCKUUAZZMMPV3S5VGHLANCNFSM4XCYDEKA> .

[mweststrate]

Looking great! Thanks for making these changes. As a really nit you might want to mention in the changelog that it will reduce memory usage of MobX in production builds, which is why some projects might be interested in upgrading :)

[mweststrate]

Fun, _mapid was already unused since 2016 :) 6bf6da9

[iChenLei]

Fun, _mapid was already unused since 2016 :) 6bf6da9

That's funny, I was a sophomore college student in 2016, after 5 years I delete it. hah O(∩_∩)O~~

[iChenLei]

Looking great! Thanks for making these changes. As a really nit you might want to mention in the changelog that it will reduce memory usage of MobX in production builds, which is why some projects might be interested in upgrading :)

So you means add reduce memory usage info to changset ? Sir

[iChenLei]

Sir, I think I should do the same work for mobx4and5 branch. I will create anthor new pull request for it.

[danielkcz]

Sir, I think I should do the same work for mobx4and5 branch. I will create anthor new pull request for it.

Publishing older versions has to be done manually. I don't think this change can be classified as "critical bugfix". It's not worth the hassle imo and as Michel said, people might be at least motivated to upgrade to V6 which is our goal here.

[iChenLei]

Publishing older versions has to be done manually. I don't think this change can be classified as "critical bugfix". It's not worth the hassle imo and as Michel said, people might be at least motivated to upgrade to V6 which is our goal here.

Ok , I got it. 😄

[mweststrate]

@iChenLei yes that was about the changeset indeed :)

[danielkcz]

@iChenLei Perhaps be more explicit in the changeset and mention the removal of mapid_ specifically in case someone was crazy enough to use that for something.

[danielkcz]

Thanks, let's merge.

[urugator]

Is it an intention to pass __DEV__ ? dynamic : static to every createAtom/new ObservableValue invocation instead of just ignoring the passed value inside Atom constructor?
I am asking because I think you have missed few places and also you do ignore name in ComputedValue constructor (as opposed to Atom/ObservableValue).