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

Dual vend as ES/CommonJS? #26

Closed
vcschapp opened this issue Jan 9, 2025 · 11 comments
Closed

Dual vend as ES/CommonJS? #26

vcschapp opened this issue Jan 9, 2025 · 11 comments

Comments

@vcschapp
Copy link
Contributor

vcschapp commented Jan 9, 2025

I've been playing with this library for a few days and it just happens to be exactly what I need, it's basically an amazing stroke of luck for me that you happened to build this just when you did! Thank you. 🙏

When I tried integrating this into an older TS application that has to vend CommonJS modules, not ES modules, I realized that it borders on impossible to make a CJS module import an ES module in a usable way due to async poisoning... I'm at the point where I'm unsure if I should try to hack some kind of de-async solution or just stop using the library...

Would you be opening to dual-vending the package as both CJS and ES modules so it can be used from CJS context without async poisoning?

@antfu
Copy link
Collaborator

antfu commented Jan 9, 2025

Are you aware of https://joyeecheung.github.io/blog/2024/03/18/require-esm-in-node-js/ and do you consider it an option? With that I see there are even less reasons for new ESM-only package to bring back CJS support.

@slevithan
Copy link
Owner

I'm open to it (at least discussing more) but would much prefer to avoid it, since providing a CJS bundle would require both long-term support and changes to how types are exported. Can you use @antfu's suggestion?

Aside: If you're able to share, I'd love to hear about how you intend to use oniguruma-to-es in your project. 😊

@vcschapp
Copy link
Contributor Author

vcschapp commented Jan 9, 2025

Are you aware of https://joyeecheung.github.io/blog/2024/03/18/require-esm-in-node-js/ and do you consider it an option? With that I see there are even less reasons for new ESM-only package to bring back CJS support.

@antfu I don't think it's an option for me at the moment, but if you spot an erroneous assumption in my reasons below let me know. These are my reasons.

  1. I need things pretty much now, and I have dependencies downstream of me so I can't just start upgrading everything, as that will break them.
  2. That features seems to still be experimental but in any event is in a Node version I can't currently run on. (I am pinned to Node 18 at the moment.)

Total aside, and I'm no expert on the JS ecosystem, being something of a generalist, but my impression of the CJS/ES issue is that it imposes a very large cognitive and time burden on developers who just want to build stuff but instead find themselves caught in the middle of a battle about deep implementation details of competing module systems that is mostly irrelevant to them... I love that people like @joyeecheung are fighting to reduce the confusion and increase compatibility things over, but it still feels like the ES system isn't mature enough to depend on it exclusively...

@vcschapp
Copy link
Contributor Author

vcschapp commented Jan 9, 2025

@slevithan thanks for the reply! I don't think that @antfu's solution works for me but I'm glad to learn about it, as I hadn't previously known.

Regarding your question on usage:

Aside: If you're able to share, I'd love to hear about how you intend to use oniguruma-to-es in your project. 😊

I'm writing some AWS CDK constructs that quietly produce Fluent Bit configuration files in the background.

  • Because Fluent Bit uses Onigmo regular expressions in Ruby compatibility mode, users of my library need to provide Ruby-compatible regular expressions.
  • Just do to the nature of the library, there's quite a long process between where my users provide their CDK configuration and when they will ultimately discover whether their Fluent Bit configuration works correctly, so I'm trying to front load as much validation as possible so things can fail fast, keeping that edit/compile/test loop as tight as possible.

So I'm using a few different features from your library:

  1. Acceptable consistency between Fluent Bit and Oniguruma syntax.
  2. Ability to evaluate the regular expressions during the CDK build.
  3. Ability to traverse the AST and pull out the named capture groups, which lets me validate a few other subtle aspects of the Fluent Bit configuration during the CDK build.

@slevithan
Copy link
Owner

slevithan commented Jan 10, 2025

Your validation use case is interesting. I've updated the readme intro to mention it.

I think I'll cave and add a CommonJS bundle in the next non-bugfix version. That will probably also resolve #7.

In the meantime, you can fork and follow the setup from Regex+'s package.json, including exports and the bundle:cjs and postbuild scripts.

@vcschapp
Copy link
Contributor Author

vcschapp commented Jan 10, 2025

Amazing. Thank you!

Not to push too hard, but do you have a rough timeframe in mind for the next non-bugfix version? (Just for my info so I can plan some next steps on my side.)

@vcschapp
Copy link
Contributor Author

For what it's worth, I opened a PR: #28.

I tried to copy your example on https://github.com/slevithan/regex, so hopefully it's a model that you find useful, but I'm happy to make any edits requested, and won't take any offense if you have to throw the whole thing out and redo it differently!

@vcschapp
Copy link
Contributor Author

@slevithan Thanks for merging the PR!

JFYI in testing the CJS paths locally, I am getting a bunch of ERR_PACKAGE_PATH_NOT_EXPORTED errors like these ones:

Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './internals' is not defined by "exports" in /path/to/my/local/oniguruma-to-es/node_modules/regex/package.json
    at exportsNotFound (node:internal/modules/esm/resolve:314:10)
    at packageExportsResolve (node:internal/modules/esm/resolve:605:13)
    at resolveExports (node:internal/modules/cjs/loader:638:36)
    at Function._findPath (node:internal/modules/cjs/loader:743:31)
    at Function._resolveFilename (node:internal/modules/cjs/loader:1230:27)
    at Function._load (node:internal/modules/cjs/loader:1070:27)
    at TracingChannel.traceSync (node:diagnostics_channel:322:14)
    at wrapModuleLoad (node:internal/modules/cjs/loader:217:24)
    at Module.require (node:internal/modules/cjs/loader:1335:12)
    at require (node:internal/modules/helpers:136:16) {
  code: 'ERR_PACKAGE_PATH_NOT_EXPORTED'
}

I was successful in fixing the first error in package regex by updating the "./internals" section of the "exports" clause to include a require clause, and then ran into the same error in regex-recursion.

I'm not enough of an expert on the ecosystem to understand whether we need to make these changes also. WDYT?

@slevithan
Copy link
Owner

slevithan commented Jan 14, 2025

@vcschapp A few follow-ups were needed, which I've now committed. This included avoiding externalizing the dependencies for the CJS bundle since some of the dependencies are only available as ESM, as you noted.

Does it work now, with 3295402 included?

If you confirm that it's working, I'm ready to release.

@slevithan
Copy link
Owner

Published in v2.0.0.

@vcschapp
Copy link
Contributor Author

@slevithan Hi Steven, I took a while to reply because I had to wait for an internal package repo to sync up with the public NPM repo, but I have v2.0.0 now and can confirm it's working as expected. Thank you so much.

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

3 participants