Rollup

[brettz9]

trafficstars

• Enhancement: Add Rollup ES distribution (with minification and sourcemaps)
• Enhancement: Add module to package.json
• npm: Remove browserify in favor of Rollup

As discussed in #234

[brettz9]

Btw, before tackling documentation, should xregexp-all.js at least be deprecated though, given that the PR puts UMD versions in dist (along with the new ES6 versions)?

[brettz9]

Also, I added dist to package.json's files property, as lib and xregexp-all.js were already there, but if each user is getting this run in a build step during install, shouldn't none of these (dist, lib, xregexp-all.js) be specified there?

[brettz9]

I've updated the branch.

[josephfrazier]

Thanks, it looks like the changes so far leave lib/ as it was on the master branch, so we know that people who npm install xregexp and use a bundler that ignores the "module" field of package.json shouldn't be affected by this. I assume that "module"-aware-bundler users also wouldn't see any changes in behavior, given the maturity of Rollup.

However, there's one problem that I noticed with xregexp-all.js: It no longer sets the global XRegExp variable when loaded in a browser. To see this, you can open the tests/index.html file after running the build script. On master, this works because of browserify's --standalone option, so Rollup should do a similar thing for this file. Once that's fixed, I think this will be ok to merge.

Also, if you could make these and further changes as separate commits, rather than force-pushing the branch, it'll be easier to keep track of them. They can be squashed by GitHub upon merging.

Thanks!

[brettz9]

Apologies--Rollup had actually been handling that aspect, but an error was preventing it from being set up. Now fixed with those HTML browser tests confirmed as running (along with the Node ones).

[josephfrazier]

Oh, that explains it, Babel's add-module-exports was conflicting with Rollup's implementation, thanks for the fix! I decided to make Rollup's modifications of the Babel config a little more explicit and flexible, but this LGTM! I'll give it a little more time for @slevithan or @mathiasbynens to take a look before merging.

---

Hmm, wait: It looks like this actually increases the size (measured with wc -c) of xregexp-all.js from 240800 bytes (on the master branch at f6e32c6280d5eed963d104794c29cfc990d8adbb) to 244037 bytes. Are you seeing the same thing?

[brettz9]

See my latest commit. Now down to 236223 bytes (without minifying).

[josephfrazier]

Thanks, I added a couple of changes to prevent unnecessary files from being published and to simplify the Rollup config. Details are in the commit messages.

[brettz9]

Apologies for the delayed reply. Looks good. The only concern I have is that if you want to deprecate using xregexp-all.js (so there is consistency in using the dist directory), I think it'd be a good idea to keep in that non-minified build (i.e., a simple call to getRollupObject(),) so people can be advised to start using it.

[jsg2021]

I've submitted an alternate to this PR (#282) In that PR, we use rollup to generate the ./xregexp-all.js (umd) I made the package point module aware loaders (rollup/webpack) to the ./src instead and cjs loaders (node) point to the umd... this should cut the publish size in half and simplify the package. No need to deprecate the xregexp-all, and no external change other than size 😊

[brettz9]

That would work for me, but the reason I took the approach of a separate dist is that there would be no future breaking changes if xregexp needed to add dependencies; in such a case, src would no longer work out of the box, e.g., browsers could not load it as a module. And since a dist folder was being added, it makes sense to be able to look for the UMD build in the same place as well. If the project is confident they won't add dependencies, then this precaution isn't necessary.

[jsg2021]

If the project is using non-standard features or newer syntax not in node8 then yes, we’d need to run it through babel. In which case, I’d recommend not making a dual-entry package and just publish the umd.

The rollup config can output all formats we would want. cjs/umd/esm.

[brettz9]

So you would require that consumers then do their own rolling up, albeit with a provided config?

While that is better than nothing, I personally prefer the "pre-built binaries available" approach, with module pointing to a usable file. ESM is not only useful for projects with existing build routines, but also for demos and applications which deliberately avoid an extra build step for rapid development.

[brettz9]

There is the Babel need too, but I was referring to the fact that browsers wouldn't recognize paths for dependencies you might add like import 'some-npm-module'. If you were importing dependencies in this manner in source, module would not point to a file that was usable (unless users did their own rolling up).

[jsg2021]

I think you misunderstood me. I was saying the umd is the pre-built binary that serves browsers (via script tag) and commonjs. Newer projects using webpack/rollup would just work as they do now.

[brettz9]

But browser projects which don't use Rollup (and don't want an extra build step) and wish to use modules for modularity (avoiding polluting their global HTML with dependency chain information or their JavaScript with global variables) would not be able to do so without an ESM build.

[jsg2021]

Which is why i conceded to your point several comments ago. We just have rollup produce a umd and an esm. Remove src as the module entry point.

[brettz9]

Ok, I see, my apologies then. Yes, that sounds good to me.

[josephfrazier]

Hey @brettz9 and @jsg2021, I'm starting to look back into using rollup. I've tried to update both this PR and https://github.com/slevithan/xregexp/pull/282 against the master branch, but I'm not sure one I should continue on. Do you both each prefer your own branch, or has one of you convinced the other their approach is better?

EDIT: I'm currently getting this error when I run npm install (which does a build) on this branch:

Error: Cannot find module 'babel-preset-env' from '/Users/josephfrazier/workspace/xregexp'
- Did you mean "@babel/env"?
    at Function.resolveSync [as sync] (/Users/josephfrazier/workspace/xregexp/node_modules/resolve/lib/sync.js:89:15)
    at resolveStandardizedName (/Users/josephfrazier/workspace/xregexp/node_modules/@babel/core/lib/config/files/plugins.js:101:31)
    at resolvePreset (/Users/josephfrazier/workspace/xregexp/node_modules/@babel/core/lib/config/files/plugins.js:58:10)
    at loadPreset (/Users/josephfrazier/workspace/xregexp/node_modules/@babel/core/lib/config/files/plugins.js:77:20)
    at createDescriptor (/Users/josephfrazier/workspace/xregexp/node_modules/@babel/core/lib/config/config-descriptors.js:154:9)
    at /Users/josephfrazier/workspace/xregexp/node_modules/@babel/core/lib/config/config-descriptors.js:109:50
    at Array.map (<anonymous>)
    at createDescriptors (/Users/josephfrazier/workspace/xregexp/node_modules/@babel/core/lib/config/config-descriptors.js:109:29)
    at createPresetDescriptors (/Users/josephfrazier/workspace/xregexp/node_modules/@babel/core/lib/config/config-descriptors.js:101:10)
    at presets (/Users/josephfrazier/workspace/xregexp/node_modules/@babel/core/lib/config/config-descriptors.js:47:19) {
  code: 'MODULE_NOT_FOUND'
}

[jsg2021]

If my pr addressed @brettz9 's concerns (I think it did) then you may find mine desirable since it is setup to make rollup output all your various bundle formats (umd/esm/cjs) in one go.

[jsg2021]

But I don't really have a preference. Not having any dependencies on babel or core-js would be my only real desire.

[brettz9]

I don't have much time to look at this currently, but I expect the other PR should be fine (though unlike per https://github.com/slevithan/xregexp/pull/282/files#issuecomment-729919943 , I do hope it will keep its ESM build so that browser demos could use the ESM build directly from node_modules.

The PR should probably switch to the currently maintained @rollup/plugin-eslint and @rollup/plugin-babel. I think the latter's API changes to babelHelpers: 'bundled', however.

[slevithan]

@josephfrazier Do you expect to include this PR or #282 in v5.0.0? There's nothing currently holding back that release, but it might be nice to include build changes at the same time.

[josephfrazier]

Thanks for chiming in, @jsg2021 and @brettz9. I can't really give an estimate of when I'll be able to get this done, but I am interested in doing so.

@slevithan, I see you merged some more bug fixes, and I wouldn't want to hold up the v5.0.0 release on account of this, so I'll try to get it out today.

EDIT: I also think that this build process change possibly shouldn't be breaking, so we may not need a v6.0.0 for it.