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

Provide an option to overwrite destination empowered assertion library #84

Open
rmdm opened this issue Apr 12, 2017 · 2 comments
Open

Comments

@rmdm
Copy link

rmdm commented Apr 12, 2017

It seems like there is a great infrastructure around power-assert. More than this you provide a way for other assertion libraries to empowerment which is awesome! Nevertheless it seems like most of the instrumentors are power-assert-centric a bit (not a big surprise though =)). What I mean by that is implicit rewriting of require('assert') with hardcoded require('power-assert'). Would it be possible for all of these great instruments to provide an option to overwrite default power-assert with any other empowered assertion library? Alternatives that I see for such libraries either to ask their users to explicitly require them or maybe to recreate some of the instrumentors which seems the wrong way. What is your thoughts on this?

UPD Have proposed initial changes to the root instrumentors module via PR teppeis/empower-assert#3

@twada
Copy link
Member

twada commented Apr 19, 2017

@rmdm Thank you for your suggestion. Your suggestion makes sense.

You are pointing implicit rewriting of require('assert') with hardcoded require('power-assert').
Yes. It's better to make destination library customizable.

However, it's a bit complicated problem. Let me explain.
power-assert system consists of two parts, the Transpiler side and the Runtime side, and the library power-assert acts as a facade module of the many tiny runtime side modules.

Therefore, it's a bit difficult to satisfy requirements for the Runtime side of power-assert system without power-assert module. The requirements are:

  • 100% API compatible with assert (no more, no less)
  • accept and interpret captured values embedded by transpiler side, and format them (you can do it manually with empower-core and power-assert-context-formatter)
  • (of course) the library should be in node_modules

I think it's reasonable to provide an option to stop implicit rewriting of require('assert'). (Hmm, No, it's not reasonable since the normal assert module does not satisfy the 2nd spec above)
On the other hand, if I provide an option to customize destination library, it's good to have, but also a bit difficult to use it correctly.

I think your suggestion is nice and it's easy to implement with your proposed PR (thanks!).
I concern about misuse of the option.

What do you think?

@rmdm
Copy link
Author

rmdm commented Apr 20, 2017

@twada Thank you for your points. Indeed, consistent and solid API is important aspect, and I see now that introduction of such an option would make it more fragile. So, I would not insist on anything and leave the decision up to you.

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

No branches or pull requests

2 participants