Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define action name to be as the function name #2262

Merged
merged 3 commits into from
Jan 24, 2020

Conversation

nadavkaner
Copy link
Contributor

@nadavkaner nadavkaner commented Jan 24, 2020

Thanks for taking the effort to create a PR!

If you are creating an extensive PR, you might want to open an issue with your idea first, so that you don't put a lot of effort in an PR that wouldn't be accepted. Please prepend pull requests with WIP: if they are not yet finished
PR checklist:

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

Feel free to ask help with any of these boxes!

The above process doesn't apply to doc updates etc.

@nadavkaner
Copy link
Contributor Author

Do I need to make these changes also in v4 folder?

test/v5/base/action.js Outdated Show resolved Hide resolved
@danielkcz
Copy link
Contributor

Do I need to make these changes also in v4 folder?

Once the v5 is polished, the v4 will be needed as well for sure.

src/v5/core/action.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@danielkcz danielkcz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but perhaps add one more test where name would be created within the test as non-configurable? To avoid regressions in the future.

@nadavkaner
Copy link
Contributor Author

nadavkaner commented Jan 24, 2020

How I can change functions name to be non-configurable inside the test?
I tried something like this Object.defineProperty(Function, "name", { configurable: false }), but it doesn't seems to work

@danielkcz
Copy link
Contributor

danielkcz commented Jan 24, 2020

I suppose you cannot make that particular property non-configurable, but let's "cheat" and use Object.seal which makes everything ... well, sealed :)

Oh, that won't work because you are creating a new object for that check every time. That's not ideal, but I cannot think of a different approach.

It could work with Object.seal(Function.prototype), but there is no way to "unseal".

@nadavkaner
Copy link
Contributor Author

nadavkaner commented Jan 24, 2020

With Object.seal I can seal only specific function instance and not every function instance that being created which this is what I need in this case, am I missing something here?
It doesn't work with Object.seal(Function.prototype) either :\

@danielkcz
Copy link
Contributor

You not missing anything, I haven't realized how is the check done. I suppose it would have to be enough. Complicating the code further for the sake of tests is probably not worth it.

@nadavkaner
Copy link
Contributor Author

I agree :)

@danielkcz danielkcz merged commit 6e96eaf into mobxjs:master Jan 24, 2020
@nadavkaner nadavkaner deleted the define-action-name branch January 24, 2020 22:20
@danielkcz
Copy link
Contributor

Oh, btw, next time please don't tick the box that you have updated changelog. Clearly you did not and I forgot to check. Nevermind now, I will add it there directly

@elektronik2k5
Copy link
Contributor

This is awesome! BTW, why not do this in production mode too?

@danielkcz
Copy link
Contributor

@elektronik2k5 That would work only if the action name is passed as a string in the first arg. Function names are usually minified. It would be inconsistent. Besides, this is intended for debugging purposes mostly.

@elektronik2k5
Copy link
Contributor

@FredyC, digging a bit further into the code, I see it isn't that simple. I understand that and am not challenging it any further right now - and definitely not as part of this PR.

However, I still want to address some of your assertions and perhaps we can improve it in the future:

Function names are usually minified.

You're right: function names are usually mangled. Practically always in browsers and often in node packages which go through a build/compilation step.

However, that is not always the case:

  • I always configured the minifiers in my projects to not mangle function names cause I value readable stack traces in production much more than the negligible size decrease which mangling produces. You can have minification and keep readable names thanks to gzip/brotli. And it doesn't negate all other minification tricks. :)
  • Thanks to all modern browsers supporting ESM, dynamic import() hitting stage 4, HTTP >= 2 and Brotli we have things like https://www.snowpack.dev/, where we're able to go back to working like once: via <script> tags and without bundling. It is a valid approach and a breath of fresh air in light of our currently tooling-heavy workflows.
  • In node, there's absolutely no need to minify anything, and we know mobx is used in node too.
  • Due to the fact that every application's target environment is different and package sources are written in different languages using different standards, there's talk of reversing the current standard of shipping ES5 packages and instead:
    • Shipping different target environment bundles: ES5, ES2015 without ESM, ESM + latest. Some packages already do this and bundler support is in place.
    • Ignoring bundled package artifacts altogether and having the bundler take care of compilation, polyfilling, minification, etc of raw source code. React Native does this, for example.

It would be inconsistent.

Only if we let it be inconsistent.

this is intended for debugging purposes mostly

In my view, debugging isn't a nice to have, but a first class feature of any tool/package I consider using.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants