Fix minification issue / make implementation more solid

[lukaselmer]

• Fixes
  • Wrong event names after uglify-ing the code #406
  • Client won't work after minification is done.  #492
  • Raise the correct event name #407
• Conforms with the eslint rules of the current master branch.
• The code changes are covered by the tests:

With the refactored implementation, all tests are green:

Changing return sendResult('searchEntry', msg) to return sendResult('somethingSilly', msg) results in 14 asssertation errors:

Changing return sendResult('searchReference', msg) to return sendResult('somethingSilly', msg) results in 2 asssertation errors:

[jsumners]

A few things:

1. Thank you for taking the time to contribute. It is appreciated.
2. This unlikely to be an issue in the next branch. If it is, the SearchEntry object will come from https://github.com/ldapjs/messages and the check should be Object.prototype.toString.call(msg) === '[object LdapMessage]' && msg.type === 'SearchEntry'.
3. If we are going to publish this against the v2 release line (and I am really hesitant to make any further releases on that line), then this PR should target the v2 branch. I am not willing to make any further changes to the master branch until I am able to merge the next branch into it.
4. This PR would need a corresponding unit test to cover it before it can be accepted.

[lukaselmer]

Thank you for the quick response.

This issue is (still) causing a headache since 6 years. Unfortunately it's also a really sneaky issue, which can take hours to understand and find the root cause. Worst of all, it only shows very late in the development process (only in the integration tests, where the code is minified).

The fix is simple and clean IMO. It makes the code easier to read and understand.

4. This PR would need a corresponding unit test to cover it before it can be accepted.

I understand that you'd like to have a regression test for this. I thought about it for quite some time on how to write one, but I didn't come up with a solution to emulate a different class name for SearchEntry or SearchReference, except actually renaming the classes (which seems silly). Or to somehow overwrite the name of the constructor (I don't think that's a good idea, if it would even be possible?). Or somehow minify the code before running the tests...? Do you have a suggestion on how to test it otherwise?

As already shown, the current tests already cover the implementation.

2. This unlikely to be an issue in the next branch. If it is, the SearchEntry object will come from https://github.com/ldapjs/messages and the check should be Object.prototype.toString.call(msg) === '[object LdapMessage]' && msg.type === 'SearchEntry'.

I'm not sure, but I think Object.prototype.toString.call(msg) === '[object LdapMessage]' would suffer from the same issue. But I didn't find it in the next branch. Instead I found

} else if (Object.prototype.toString.call(options.filter) !== '[object FilterString]') {

which could be replaced with options.filter instanceof FilterString => I'm making a PR for this on the next branch, hope you don't mind :)

What I found on the next branch regarding the type getter LGTM and makes the code more solid.

3. If we are going to publish this against the v2 release line (and I am really hesitant to make any further releases on that line), then this PR should target the v2 branch. I am not willing to make any further changes to the master branch until I am able to merge the next branch into it.

OK, so I change the target to the v2 branch 🤞

[jsumners]

As already shown, the current tests already cover the implementation.

The current tests pass. Therefore, the "issue" being fixed is not covered by the tests. A test should be added that fails without the change.

But I didn't find it in the next branch. Instead I found

As I said, it will be replaced by the currently in-development package. I do not have @ldapjs/messages complete yet, nor have I attempted to integrate it into this code base yet. There will be significant changes when I do, though.

which could be replaced with options.filter instanceof FilterString => I'm making a PR for this on the next branch, hope you don't mind :)

An instanceof check will not work for the exact same reason outlined in fastify/fastify-error#86 (comment)

[lukaselmer]

I changed the target branch to v2.x

The current tests pass. Therefore, the "issue" being fixed is not covered by the tests. A test should be added that fails without the change.

Agree.

At least the tests ensure that what was working before still works. And we know that it actually fixes a bug? Even if it wouldn't, would the code be easier to read and understand?

Anyways - do you have a suggestion on how to add such a test? I don't know how to do that without big changes 🤔

will be replaced

Ok, got it.

An instanceof check will not work for the exact same reason outlined in fastify/fastify-error#86 (comment)

Surprising! 🤔

Hmm. I see that you already handled the issue by overwriting toStringTag

Sorry for the false positive

[lukaselmer]

I found another solution with even less changes. However, I personally still prefer the changes proposed in this PR:

e1146da

I also tried renaming the classes internally. I don't think that's a good idea, but it would test the fix:

30bde8a

What do you think?

[jsumners]

I found another solution with even less changes. However, I personally still prefer the changes proposed in this PR:

e1146da

This seems equivalent, but I cannot say that for sure.

I also tried renaming the classes internally. I don't think that's a good idea, but it would test the fix:

30bde8a

This is not accurate. The current package provides the message "classes" for anyone to use.

At least the tests ensure that what was working before still works.

Correct.

And we know that it actually fixes a bug?

Does it? I have no idea without a test showing what is being fixed.

Even if it wouldn't, would the code be easier to read and understand?

Possibly? This seems like a weak argument when the stated intent is to solve some failure of the code.

[lukaselmer]

Let's get one thing straight: code which relies on constructor.name is fragile, because constructor.name changes when minifying the code. Do you agree?

• https://www.beyondjava.net/constructor-name
• https://dev.to/gaurangdhorda/constructor-name-is-not-working-in-production-3ik4

In general, relying on a specific class name (which is a specific implementation) violates LSP:

• https://en.wikipedia.org/wiki/Liskov_substitution_principle

Someone probably already noticed, that depending on the class name is not a good idea, and added a type accessor 🤔

This is not accurate. The current package provides the message "classes" for anyone to use.
Does it? I have no idea without a test showing what is being fixed.

(Some / The most?) popular minifiers will minify the function names / class names by default:

• https://github.com/terser/terser#minify-options (see keep_classnames / keep_fnames)
• https://esbuild.github.io/api/#minify

30bde8a emulates that the classes/functions are renamed => it tests that the code breaks in that case. There's no other test which checks that the message classes are named the way they are. But that doesn't mean nobody outside of this package will use these classes, and use constructor.name on it, so it may not be a good idea to rename them. Of course, in these rare cases the minification wouldn't work for the same reason that it doesn't work for this library.

Then, if we apply one of the solutions (the one in this PR, or e1146da), it will fix the test.

But does it really fix a bug?

I understand that you are hesitant to merge this, and I understand that you want to have this bug first confirmed, then resolved, and then kept it that way. And the best way to guarantee that is to have a test. I know, 100% agree, I'm with you.

But people are having real-life issues here. Otherwise there wouldn't have been all these issues / PRs. So we know there's a bug. We just don't know how to test it in an automated way, except writing a program, then minify it's dependencies, and then observe that it stops working. And that "test" would be out of proportion, wouldn't it? I mean technically we could write such a test, but is it necessary?

I've tried to write a unit tests, and I couldn't manage it, because everything is so encapsulated that the only way I found to test it is to manually rename the class, which I did in 30bde8a. I'm not proud of that, and I'd prefer to have a better way.

The good thing: the new code is tested. And it is simpler. Clearer. More direct. Less magical. So in the very worst case, it does nothing. Right?

Possibly? This seems like a weak argument when the stated intent is to solve some failure of the code.

Hmm. It's a completely different argument, agree. And yes, a unit test which shows the system failing would be a very strong argument. But by no means is this argument weak: code which is easier to understand and reason about is less likely to contain a bug - especially when we know that there is a bug in the current implementation :)

[jsumners]

Let's get one thing straight: code which relies on constructor.name is fragile, because constructor.name changes when minifying the code. Do you agree?

No. If your tools are changing the guarantees of the language then your tools are to blame, not the code.

In general, relying on a specific class name (which is a specific implementation) violates LSP:

• https://en.wikipedia.org/wiki/Liskov_substitution_principle

Uh, sure. As the code base is updated to use the new independent modules this will be resolved. Probably.

Someone probably already noticed, that depending on the class name is not a good idea, and added a type accessor

I have no idea why the type accessor was added to these objects. I am merely keeping it in my current work as a matter of compatibility.

(Some / The most?) popular minifiers will minify the function names / class names by default:

And here's the statement you will not like: I do not care about minifiers, or transpilation of any sort, in the slightest.

I understand that you are hesitant to merge this, and I understand that you want to have this bug first confirmed, then resolved, and then kept it that way. And the best way to guarantee that is to have a test. I know, 100% agree, I'm with you.

Good.

But people are having real-life issues here. Otherwise there wouldn't have been all these issues / PRs. So we know there's a bug.

This is what a seemingly small subset of users claim. If it were a widespread problem there would certainly be much more discussion and interest in fixing it.

We just don't know how to test it in an automated way, except writing a program, then minify it's dependencies, and then observe that it stops working. And that "test" would be out of proportion, wouldn't it?

No one said writing such a test would be easy. The "proportion" of the test is related to the problem.

I've tried to write a unit tests, and I couldn't manage it, because everything is so encapsulated that the only way I found to test it is to manually rename the class, which I did in 30bde8a. I'm not proud of that, and I'd prefer to have a better way.

Write an integration test. But renaming the objects as you have linked to multiple times is not the right way to do it. As stated before: they are public objects.

The good thing: the new code is tested.

No, it isn't. It is not tested according to the stated purpose of the change.

especially when we know that there is a bug in the current implementation

Again, we do not know there is a bug when there is not a test case to prove it.

---

Here's the short of it: this project was never designed to be used in the manner in which you are using it. It is a Node.js module designed to be run on top of the Node.js runtime. Such modules have zero need to be transpiled. If you intend to transpile the code, then it is up to you to configure your tooling to not break your dependencies.

That being said, we want contributors to this project and for this project to be a product of the contributors's work based upon their needs. But we need those contributors to support their work, and the minimum support they can provide is to provide a guarantee that their contributions do what they claim to do. This is done by including at least one test that proves the contribution.

This PR could be the only thing you ever add to this project. If it is merged without a covering test, then in 6 months or 2 years from now when the code is changed again there will be no way to know if the problem has been avoided or reintroduced. And without you around to review every change, it is likely to be the case that the issue will be reintroduced.

[lukaselmer]

I give up

[jsumners]

⚠️ This issue has been locked due to age. If you have encountered a recent
problem that seems to be covered by this issue, please open a new issue.

Please include a minimal reproducible example
when opening a new issue.