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

feat: be possible to skip re2 #656

Merged
merged 1 commit into from
Aug 16, 2023
Merged

feat: be possible to skip re2 #656

merged 1 commit into from
Aug 16, 2023

Conversation

Kikobeats
Copy link
Member

closes #630

@gajus
Copy link

gajus commented Aug 16, 2023

Amazing! RE2 is causing so many issues in our setup. Good riddance.

@Kikobeats Kikobeats merged commit 7dd62e7 into master Aug 16, 2023
31 checks passed
@Kikobeats Kikobeats deleted the re2 branch August 16, 2023 20:06
@gajus
Copy link

gajus commented Oct 13, 2023

@gajus
Copy link

gajus commented Oct 13, 2023

In case it is not clear what I mean, by being a direct dependency of that package, it still gets pulled in:

dependencies:
@contra/integrations link:../integrations
├─┬ metascraper 5.37.1
│ └─┬ @metascraper/helpers 5.37.1
│   ├── re2 1.20.3
│   └─┬ url-regex-safe 4.0.0
│     └── re2 1.20.3 peer
├─┬ metascraper-description 5.37.1
│ └─┬ @metascraper/helpers 5.37.1
│   ├── re2 1.20.3
│   └─┬ url-regex-safe 4.0.0
│     └── re2 1.20.3 peer
├─┬ metascraper-image 5.37.1
│ └─┬ @metascraper/helpers 5.37.1
│   ├── re2 1.20.3
│   └─┬ url-regex-safe 4.0.0
│     └── re2 1.20.3 peer
└─┬ metascraper-title 5.37.1
  └─┬ @metascraper/helpers 5.37.1
    ├── re2 1.20.3
    └─┬ url-regex-safe 4.0.0
      └── re2 1.20.3 peer

@Kikobeats
Copy link
Member Author

Kikobeats commented Oct 14, 2023

That's intentional; you can skip it from execution time, but it's always present during installation.

re2 exists for a reason, and I'm not going to lower the security of this library and remove it, sorry. There are other mechanisms to avoid installing it in case you don't want it explicitly you don't want.

I am happy to accept a PR improving this workflow 🙂

@gajus
Copy link

gajus commented Oct 14, 2023

re2 exists for a reason, and I'm not going to lower the security of this library and remove it, sorry. There are other mechanisms to avoid installing it in case you don't want it explicitly you don't want.

Such as?

@gajus
Copy link

gajus commented Oct 17, 2023

@Kikobeats Just wanted to see if you have input here. I am having with workarounds, just not familiar with such.

@Kikobeats
Copy link
Member Author

Kikobeats commented Oct 18, 2023

With pnpm.io/package_json#pnpmoverrides you have granular control for whatever dependency 🙂

@gajus
Copy link

gajus commented Oct 20, 2023

I don't follow.

overrides will only allow me to change version or resolve re2 to another dependency. There is no way to remove it.

@gajus
Copy link

gajus commented Oct 20, 2023

Just realized I can pnpm patch re2 itself, remove the install action, and replace module.exports with RegExp.

Hopefully this will save time to others:

diff --git a/package.json b/package.json
index 16a3be6b9af20a0f876df7489c5d8b650f6c177a..a9946f3f97ba4c0d165dceee668aae53d8adf568 100644
--- a/package.json
+++ b/package.json
@@ -24,7 +24,8 @@
     "test": "node tests/tests.js",
     "ts-test": "tsc",
     "save-to-github": "save-to-github-cache --artifact build/Release/re2.node",
-    "install": "install-from-cache --artifact build/Release/re2.node --host-var RE2_DOWNLOAD_MIRROR --skip-path-var RE2_DOWNLOAD_SKIP_PATH --skip-ver-var RE2_DOWNLOAD_SKIP_VER || npm run rebuild",
+    "original-install": "install-from-cache --artifact build/Release/re2.node --host-var RE2_DOWNLOAD_MIRROR --skip-path-var RE2_DOWNLOAD_SKIP_PATH --skip-ver-var RE2_DOWNLOAD_SKIP_VER || npm run rebuild",
+    "install": "echo 'Skipping RE2 install'",
     "verify-build": "node scripts/verify-build.js",
     "rebuild": "node-gyp rebuild"
   },
diff --git a/re2.js b/re2.js
index b6b1eb7002ef5df623190c0d8ac21e83490a1370..c3bfda18657119c36fc5de301869804bdbfe7820 100644
--- a/re2.js
+++ b/re2.js
@@ -1,37 +1,3 @@
 'use strict';
 
-const RE2 = require('./build/Release/re2');
-
-if (typeof Symbol != 'undefined') {
-  Symbol.match &&
-    (RE2.prototype[Symbol.match] = function (str) {
-      return this.match(str);
-    });
-  Symbol.search &&
-    (RE2.prototype[Symbol.search] = function (str) {
-      return this.search(str);
-    });
-  Symbol.replace &&
-    (RE2.prototype[Symbol.replace] = function (str, repl) {
-      return this.replace(str, repl);
-    });
-  Symbol.split &&
-    (RE2.prototype[Symbol.split] = function (str, limit) {
-      return this.split(str, limit);
-    });
-  Symbol.matchAll &&
-    (RE2.prototype[Symbol.matchAll] = function* (str) {
-      if (!this.global) {
-        throw TypeError('String.prototype.matchAll called with a non-global RE2 argument');
-      }
-      const re = new RE2(this);
-      re.lastIndex = this.lastIndex;
-      for (;;) {
-        const result = re.exec(str);
-        if (!result) break;
-        yield result;
-      }
-    });
-}
-
-module.exports = RE2;
+module.exports = RegExp;

@aldenquimby
Copy link
Contributor

@gajus have you published a version of this package anywhere with re2 removed? We are also running into build failures and are planning to use open-graph-scraper instead. I'd prefer to use metascraper

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.

provide a way to avoid re2 installation
3 participants