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

Mobx6 - Add action name in prod too #2343

Merged
merged 5 commits into from
Apr 25, 2020
Merged

Conversation

elektronik2k5
Copy link
Contributor

@elektronik2k5 elektronik2k5 commented Apr 25, 2020

Changes:

  • Define action.name unconditionally. Fixes Minification & debugging #2263
  • Update readme to reflect correct scripts
  • Fix prettier script to overwrite files
  • Fix .prettierignore to ignore generated assets
  • Run prettier on repo: 2 changed files

Code change checklist

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

I'm not sure about how to test for perf regressions: I ran yarn build && yarn perf before and after with these results:

Before:

yarn test:performance proxy
Completed performance suite in 60.257 sec.
64.66user 1.62system 1:00.32elapsed 109%CPU (0avgtext+0avgdata 1092404maxresident)k
1136inputs+16outputs (1major+476988minor)pagefaults 0swaps

yarn test:performance legacy
Completed performance suite in 61.295 sec.
66.08user 1.66system 1:01.36elapsed 110%CPU (0avgtext+0avgdata 1033992maxresident)k
0inputs+16outputs (0major+477497minor)pagefaults 0swaps

After:

test/perf/index.js proxy
Completed performance suite in 61.343 sec.
64.80user 1.74system 1:01.41elapsed 108%CPU (0avgtext+0avgdata 1065372maxresident)k
0inputs+16outputs (0major+484730minor)pagefaults 0swaps

test/perf/index.js legacy
Completed performance suite in 60.269 sec.
65.21user 1.68system 1:00.33elapsed 110%CPU (0avgtext+0avgdata 1062128maxresident)k
0inputs+16outputs (0major+475698minor)pagefaults 0swaps

Looks pretty much the same.

@gitpod-io
Copy link

gitpod-io bot commented Apr 25, 2020

@elektronik2k5 elektronik2k5 marked this pull request as ready for review April 25, 2020 10:08
@elektronik2k5
Copy link
Contributor Author

Should I do a few more changes (is separate PRs)? Here's a list:

  • Upgrade all deps
  • Add spell checker
  • Run tests against built package (not for perf)

@danielkcz
Copy link
Contributor

danielkcz commented Apr 25, 2020

Thanks for doing it. I am not sure you should even do other unrelated changes you did here. @mweststrate is working on that branch and if he has some unpublished changes it could cause unnecessary conflicts.

I will leave merging up to him in case there are things he wants to do differently.

Copy link
Member

@mweststrate mweststrate left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

@danielkcz
Copy link
Contributor

I guess I will do merging then :)

@danielkcz danielkcz merged commit 5164bbf into mobxjs:mobx6 Apr 25, 2020
@elektronik2k5
Copy link
Contributor Author

Thanks guys! :)

Should I do a few more changes (is separate PRs)? Here's a list:

  • Upgrade all deps
  • Add spell checker
  • Run tests against built package (not for perf)

@mweststrate what about this list?

@mweststrate
Copy link
Member

@elektronik2k5

  1. go ahead!
  2. can you clarify?
  3. yes that would be awesome, not sure how to deal with obfuscated / not-in-prod errors. Maybe those tests need to be in an if block?

@mweststrate
Copy link
Member

Unrelated, I was thinking making observableRequiresReaction the default, how'd you feel about that?

@elektronik2k5
Copy link
Contributor Author

  1. can you clarify?

Yes, here's a story: as a renown quality and grammar Nazi 😏, I review all of my client's code and catch a lot of typos. However, I once had to work with a team member who was particularly bad at spelling and got sick of pointing out obvious typos. So I decided to add a spell checker as a git hook and it worked well :). However, I also discovered that the codebase still has loads of typos which I missed.

The point is: a computer can spell check much better than any person. I've been using cspell to catch spelling errors in codebases for a few years now.
It works exactly like any other linter for files staged for a commit: fail if non English words are found. Then you can fix the spelling, ignore it once or whitelist the offending word.
cspell also has a VSCode extension which picks up the configuration from a project and integrates with the "problems" panel, just like eslint, typescript and others.

  1. yes that would be awesome, not sure how to deal with obfuscated / not-in-prod errors. Maybe those tests need to be in an if block?

Perhaps, I'll look into it.

@elektronik2k5
Copy link
Contributor Author

Unrelated, I was thinking making observableRequiresReaction the default, how'd you feel about that?

Sorry, I have no experience with any of the newer configuration options. Before MST I always worked in strict mode (is that enforceActions: 'always' nowadays?), but ever since switching to MST I haven't worked with bare mobx. :(

It is probably better to consult @Bnaya, @benjamingr and others.

@danielkcz
Copy link
Contributor

I think spellchecking in open source lib is a bit stretch. It could annoy contributors as any linter does and the benefit is very small imo. Who cares if there is a typo in a comment? Most people will understand it anyway.

@elektronik2k5
Copy link
Contributor Author

Upgrade to all deps PR is ready: #2344

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.

3 participants