Use lib library rather than xregexp-all.js

[rBurgett]

A few years ago, using a universal package declaration was sufficient so you could load a lib in either Node or the browser. But, using webpack and browserify these days makes it dangerous to use dynamic packaging. If you use the xregexp library with webpack right now, for example, it gives you a warning that you should not use compiled code. This is because each of our build processes make necessary changes to the code and if the lib uses traditional means of dynamically choosing how to expose itself, your users could potentially run into problems.

I don't know how you would want to handle it, but I would suggest that the default Node export needs to be a straight Node module which anyone can use with any build process without problem. Along with that, if someone really needs a standalone file to use in a browser, we could keep the xregexp-all.js for them. But, there are fewer and fewer people who use precompiled browser files anyway.

Every application developer has their own packaging process and this is an essential lib for application development in JavaScript.

Am I making my case well? Would you prefer to see it another way? I'm happy to keep using my own fork if you do not like the idea of changing your lib right now, but something will need to be done eventually since build processes have matured.

Thanks!

[kaidjohnson]

Can you clarify "using webpack and browserify these days makes it dangerous to use dynamic packaging"? This seems to counter the whole idea behind packaging libs for others to digest. The umd signature should be adequate...

You shouldn't need a separate lib folder. The src folder already contains an index.js that you can require if your use-case is different:

Instead of require('xregexp'); try require('xregexp/src/index.js');

[rBurgett]

There already is a separate lib folder that the library makes during the build process. The src folder is pre-Babel code. You don't want people requiring the library if it uses features which are not yet available on Node, so that's why you use Babel. My libs and the others I know use Babel to bring the code down to something usable out-of-the-box by Node. Then, the transpiled code is what we expose as the module. Then people use whatever packaging system (browserify, webpack, etc.) to package it for the browser. Out of all the libraries our current application uses, XRegExp in its current state is the only one that will not compile without throwing bright yellow warnings from Webpack, and I know I'm not the only person out there using Webpack.

I am just suggesting that the default export be a regular Node module and that anyone who is going to take the package and use it as a standalone browser script can use the browser version.

[rBurgett]

Oh, and as for what I meant about the increasing problems with UMD, I work full-time building desktop JavaScript applications and as such I often have access to both window and global and UMD can get confused in those contexts. I previously worked with a project using AMD packaging which also fell apart because I had to namespace the AMD require which broke XRegExp because it refused to expose itself as an AMD module. Back then, I didn't bring up an issue because I was very new at web programming and was just getting my footing.

But, tooling has come a long way now and the modern standard is to use a regular Node module for the export. This avoids all of those problems and other potential problems as well.

[rBurgett]

[kaidjohnson]

Good point re: babel, however the src code here appears to all be es5. Not sure where babel fits into the pipeline, although I do see it in the prebuild package.json script.

I haven't run across a scenario where a standard umd definition as provided by browserify (as used in xregexp) or webpack have troubles working inside other webpack builds. I've used xregexp in both webpack and in the browser directly and haven't noticed any problems.

[kaidjohnson]

Oh, that warning looks like it's simply a matter of telling webpack to not parse this lib via noparse option (which will speed up your build anyway)...

webpack/webpack#1617 (comment)

[rBurgett]

Yup, that sometimes works. But, xregexp is a small enough library that I just wanted to mention that it might be a good time to update the lib for modern packaging systems. Here is a comment from that same thread which makes the point better than I have.

webpack/webpack#1617 (comment)

[rBurgett]

BTW, I absolutely love xregexp and use in it literally every application I make. But, because the packaging is a little out of date, I just wanted to present the idea to you all and a PR seemed to make more sense than just creating another issue.

[kaidjohnson]

Me too. I can't take any credit for it, though; I just noticed your PR and thought I'd poke a bit to learn more :D

[kaidjohnson]

lol, looks like we're re-creating that webpack conversation... that point clarifies it a bit, but I still wonder why you would want to parse all those lib files, regardless of wether they're precompiled or not. The library author packages the library. It doesn't need additional handling, just needs to be made available in your build; re-minifying seems unnecessary as does re-compiling.

But I digress, my question would be, then, how are we leveraging babel here? The src/ code appears to all be es5 (on the surface). Any issue with simply dropping the babel bit and pointing "main" to "src/index.js"?

[rBurgett]

That's a great question. At the moment, it does look like everything would work perfectly from the src folder, but as I see it, it just makes sense to have Babel for future development. You want that build step so that anyone can write they JS without worrying about whether or not a feature is available in the different environments. The current babel config doesn't appear to actually do anything, but it's still a step worth having (in my opinion) for if anyone does need it at some point.

[josephfrazier]

Regarding Babel (since I introduced it to the build): The src/ code is all ES5 at the moment, but I'm hoping to upgrade it to ES2015+ in the future (see #174 for context)

I think @rBurgett makes great points about the benefits of having package.json main be lib/index.js, especially as someone who has also recently experienced a similar bundling issue (mozilla/webextension-polyfill#34 for anyone curious).

👍 from me :) One concern I do have is that removing lib from .gitignore will make diffs noisier, but this is balanced by the ability to npm install https://github.com/slevithan/xregexp#COMMIT_HASH and get a working version, rather than having to wait on a tagged release to npmjs.com. Given that xregexp-all.js is already tracked in the repo (and we have to make sure it's kept in sync), this doesn't really seem like much of a downside.

[slevithan]

This seems fine to me. Thanks for the PR @rBurgett, and @kaidjohnson + @josephfrazier for the discussion.

One concern I do have is that removing lib from .gitignore will make diffs noisier, but this is balanced by the ability to npm install https://github.com/slevithan/xregexp#COMMIT_HASH and get a working version, rather than having to wait on a tagged release to npmjs.com. Given that xregexp-all.js is already tracked in the repo (and we have to make sure it's kept in sync), this doesn't really seem like much of a downside.

Note that I'm very open to any changes that modernize or otherwise improve the build process, repo structure, etc. E.g. if you think that xregexp-all.js shouldn't be kept in sync in the repo, and instead only updated in a dist folder with tagged releases or something.

[josephfrazier]

Note that I'm very open to any changes that modernize or otherwise improve the build process, repo structure, etc. E.g. if you think that xregexp-all.js shouldn't be kept in sync in the repo, and instead only updated in a dist folder with tagged releases or something.

Oh, I was under the assumption that you were set on having xregexp-all.js in the repo. Personally, I'd stop tracking both it and lib/ (but still publish them to npm, of course). For folks who want a static url referencing xregexp-all.js, the https://unpkg.com/ CDN provides a way to do that (for example: https://unpkg.com/xregexp@3.2.0). We could also configure Travis CI to upload xregexp-all.js to the Releases pages. Besides reducing diff noise, another advantage of this approach is that merges and rebases become less likely to encounter conflicts (like what happened in #181 (comment)), and we would no longer need "build" commits like #185.

Again, a downside of this approach is that npm install https://github.com/slevithan/xregexp#COMMIT_HASH would stop working, but I don't how common that is, and it could be alleviated with more frequent releases. One downside particular to not tracking xregexp-all.js is that bower install xregexp might stop working with future releases (note that bower seems to have fallen out of favor in the community, and even the bower frontpage recommends yarn instead).

[slevithan]

@josephfrazier I'm down with all of that, including dropping support for bower and removing bower.json. I'm much less familiar than you guys with build/release tools though. Would you be okay with submitting the related configuration changes?

Travis CI can automatically upload assets from your $TRAVIS_BUILD_DIR to your git tags on your GitHub repository. --https://docs.travis-ci.com/user/deployment/releases/

I'm not sure what this means. Can you clarify?

[josephfrazier]

I'm not sure what this means. Can you clarify?

Sure, for example: Another project I work on (and introduced XRegExp to) uses this mechanism to make .zip files (that the build process generates) available on each github release page. Here's the .travis.yml configuration, and here's a release page with the relevant files. We could do something similar with xregexp-all.js if you'd like. Otherwise, as I mentioned in my previous comment, xregexp-all.js will still automatically be available via https://unpkg.com/xregexp.

Would you be okay with submitting the related configuration changes?

Yeah, I'd be happy to put together a PR with the non-Travis changes (since they aren't really needed). If you decide you'd like the Travis changes as well, we can work on it then.

[kaidjohnson]

After some thought on the issue, I'm not sure I fully agree with the idea of pointing "main" to src files. As an app architect, I don't want to have to compile all my third party library code on top of my own code in my builds; it's frankly more efficient, consistent, and reliable to concat independently maintained/tested packages than to re-compile code that you don't own.

I know it's after the fact, and I'm not advocating for reversing the decision; it's definitely not a deal-breaker, but I thought I'd pick some brains of the folks on this thread to get more insight into how or why this is a better approach than using webpack's noparse or your-bundler-of-choice config to avoid the warnings from which this thread was started? I haven't heard a compelling argument as to why re-compiling third-party code is better than including it but leaving it as originally authored.

[josephfrazier]

I think the idea is that bundlers default to assuming that main points to a Node-style CommonJS module, so making that be the case is what works for the greatest number of people without extra noparse/etc configuration. Note that main points to the already compiled (but not bundled) lib/ files instead of src/, so there shouldn't need to be any extra Babel/etc to make it run in Node (and only a bundler to make it run in a browser). xregexp-all.js is still present (as mentioned in #187 (comment)), so those who prefer to use that instead still can.

[kaidjohnson]

The UMD definition provides the Node-style CommonJS wrapper you mention. I believe this argument is exactly why UMD became a thing, because it's flexible across bundler formats and agnostic to whether you use a browser, CommonJS, or AMD. Basically what you're asking application maintainers to do is bundle your code for you and you're assuming that they want to bundle your code using CommonJS. I get the sense that this is a regression in flexibility. I realize they can still require the pre-bundled option, but it seems like the other way around makes more sense: provide the src when devs want to cherry-pick parts of your library, but default to the pre-bundled library code, and if a bundler throws a warning about it, it's likely an issue with the bundler (in this case webpack) and not an issue with the library. The bundler can be configured to not bother parsing your library, which not only silences the warnings but is also a performance win.

Keep in mind, too, that we're not just talking bundling here. Our bundlers also minify for us, so you're asking application maintainers to not only bundle your code, but also minify it for you, adding some serious implications to bundle times for applications that work with a lot of libraries.

To sum: I want to include your library in my bundle. I don't want to have to bundle and minify your library when I bundle my application.

[josephfrazier]

Keep in mind, too, that we're not just talking bundling here. Our bundlers also minify for us, so you're asking application maintainers to not only bundle your code, but also minify it for you, adding some serious implications to bundle times for applications that work with a lot of libraries.

Hmm, has XRegExp ever been published in a minified format? Unless I'm mistaken, it's only ever been available unminified, so I think this is a separate issue.

---

Otherwise, I'm interested to hear more about your build tooling and configuration. I'm not aware of a build tool that both relies on the package.json main field to specify the entry point, but also assumes the entry point is pre-bundled and doesn't need to be parsed. If that's not the case, and you're specifically configuring your bundler not to parse xregexp, it doesn't seem like much additional work to also configure it to use xregexp-all.js instead of file pointed to by the package.json main field.

Could you shed some light on your process? I realize that this change (and then #187) is pretty big, and it would be nice to minimize build breakage however possible.

[kaidjohnson]

So let's say the main points to xregexp-all.js in umd format. My application index.js might require('xregexp');. My bundle config would use index.js as the entry point and I would generally include all of node_modules in the noparse since those files are coming in ready-made. Now xregexp is included in my bundle, but the process of bundling can skip third-party code altogether, speeding up build time. Admittedly, this requires some effort on the bundling side of things, and we probably lose out on a few kb of treeshaking in optimal circumstances.

At the end of the day, though, it's not a huge difference either way, so I don't really want to continue bashing against it; if a library causes slowdowns during bundling or minification, the bundler config is there to allow flexibility. It's 5.5 in one hand and a half-dozen in the other -- close enough and they both work.

Thanks for the insightful feedback and conversation and for maintaining this library! It's a very useful resource and we're happy to have included it within our application!

[josephfrazier]

Oh, I see now. If you've configured your bundler to assume everything in node_modules is not to be parsed, then it makes sense why this change breaks your build. Perhaps publishing packages to npm with a UMD entry point is more common than I thought (or maybe we just use different subsets of the ecosystem). Thanks for the explanation!

I definitely understand that increased build times are a pain. You might be able to improve them by doing incremental builds using e.g. webpack's watch mode. If you're using browserify, there are a variety of options available. Other build tools have similar arrangements, I'm sure.