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

Multiple re2 installations, should be a peer dependency #2

Closed
katlim-br opened this issue Jan 3, 2022 · 4 comments
Closed

Multiple re2 installations, should be a peer dependency #2

katlim-br opened this issue Jan 3, 2022 · 4 comments

Comments

@katlim-br
Copy link

Hello

We found an error because we are using this library, and url-regex-safe at the same time, and both link to node-re2 library.

So when we run jest, we find a malloc issue. See this repo we created https://github.com/blastradius-ai/re2-malloc-error.

The solution would be to put re2 as a peer-dependency, like url-regex-safe has done it in its latest version (https://github.com/spamscanner/url-regex-safe/releases/tag/v3.0.0).

So that way we would have to install re2 ourselves, and only a single instance is created.

Hope this can be done, or if you want we can create a PR for that.

Thanks

@eliottvincent
Copy link
Member

Hey there, thanks for the report!
I'll apply the necessary changes and release a new version of the lib.

@eliottvincent
Copy link
Member

Ok so I understand the setting re2 as a peer dependency here as well would definitely fix the issue, however I don't plan to do so.
By reading the npm docs, I see that the behavior of peer dependencies is different across npm versions:

npm versions 1, 2, and 7 will automatically install peerDependencies if they are not explicitly depended upon higher in the dependency tree. For npm versions 3 through 6, you will receive a warning that the peerDependency is not installed instead.

This is quite an issue as I don't want to force host projects using this library to set re2 as a dependency. Peer dependencies were introduced to answer the use-case of plugins (https://nodejs.org/es/blog/npm/peer-dependencies/#the-problem-plugins), which is not the scope here in my opinion.

Furthermore, I see that the url-regex-safe lib has been updated and that it fixes your issue.
So I think it's safe to close this issue. Feel free to re-open the issue if needed.

@katlim-br
Copy link
Author

Indeed, you are right, our case was fixed but having two or more dependencies using node-re2 could be likely at some point, and it would hinder the usage of either library.

Probably then node-re2 has to fix this, i saw in that issue that the author was removing global state, which is highly probable the cause of this.

@eliottvincent
Copy link
Member

Agreed, but yes in any case it's the responsibility of node-re2 to fix the underlying issue.

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

No branches or pull requests

2 participants