Add module to package.json

[brettz9]

I think you want this in package.json for the sake of tools like Rollup optimizing builds.

See https://stackoverflow.com/a/42817320/271577

[brettz9]

Btw, I've also noticed your using module.exports in some files? Can this be changed to ES6 modules?

Also, can you export a browser build that is itself an ES6 module (so one doesn't need to configure plugins and can just load in the browser)?

[slevithan]

This is probably easier to discuss if you submit a pull request showing what you would change, what it would enable, and show that it still supports current methods of usage.

[brettz9]

Ok, sure, thanks, will try to see as I have time...

Btw, if I haven't shared before, my https://github.com/brettz9/regextras may suggest some benefits to adding to your available array extras (esp. some for short-circuiting and map for greater brevity)...

[brettz9]

Before going to the trouble, are you opposed to me in principle switching from browserify to Rollup to support an ES6 module distribution?

Browserify still has this issue open browserify/browserify#1186 to support ES6 modules export, while Rollup has been working nicely for such exports for some time...

[josephfrazier]

I have no problem with switching to Rollup, but note that the files currently in lib/ will probably need to stay ES5 and CommonJS, as they are the ones used in Node, which doesn't yet support ES6 modules.

EDIT: Just noticed you pushed 23bdcd4, and thought I'd add some feedback: Let's keep the package-lock.json and the ESLint changes separate from the package.json change (and from each other). If you could move those to their own PRs, they can be merged regardless of the package.json change.

[brettz9]

@josephfrazier : ~~~Regarding Rollup of Node, yes, but there is babel-register which can process ES6 modules in place as Node.~~~ (Sorry, I see you mentioned lib while I was thinking of tests.)

As far as the PR, sure, I can separate them out.

[brettz9]

Btw, the package.json linter I am using mentioned a lack of bugs, dependencies, contributors, and engines. Do you want those in the package-lock.json commit or a separate one or not at all?

[brettz9]

Also, as implicit in the name "Rollup", the process tends to package all of the files together. Is there a compelling need for preserving all of those separate lib files?

If so, can we at least move /tools within /src (adjusting relative paths), so that tools can also be expressed as ES6 Modules (and also be converted together with the rest of src)? Otherwise, I don't think adding full ES6 modules support is going to work.

[josephfrazier]

Thanks for the separate PR and the thought-provoking questions, it's nice to see such good contribution energy! I've tried to address the questions below:

Btw, the package.json linter I am using mentioned a lack of bugs, dependencies, contributors, and engines. Do you want those in the package-lock.json commit or a separate one or not at all?

I think we should start off with just adding a package-lock.json that captures the existing state of things. Separately, we can work on adding fields to package.json.

Also, as implicit in the name "Rollup", the process tends to package all of the files together. Is there a compelling need for preserving all of those separate lib files?

I believe one case is where the user doesn't want all of the XRegExp addons, so they require('xregexp/lib/xregexp.js') and then pass it to some of the addons in 'xregexp/lib/addons/', much like src/index.js does currently, but with only some of the addons rather than all of them. I'm not sure how popular this use case is, but I don't think we should eliminate it simply to provide ES6 modules alongside the ES5 ones.

If the question is "Why continue providing ES5 at all?", my opinion is that we should continue to support Node users without requiring them to use babel-register (or similar) in their build process. Here's a comment outlining the type of build output I'd personally like to see, and some pointers on how to set it up: https://www.reddit.com/r/javascript/comments/6ngjkk/how_to_distribute_library_written_with_es6/dka3g5u/

EDIT: The three.js Rollup config may also be helpful: https://github.com/mrdoob/three.js/blob/ba4489ded66212ac9e6d3017a6bb856023bd026f/rollup.config.js

If so, can we at least move /tools within /src (adjusting relative paths), so that tools can also be expressed as ES6 Modules (and also be converted together with the rest of src)? Otherwise, I don't think adding full ES6 modules support is going to work.

You can try it if you'd like, but since the /tools/ files are only used during the build process, I imagine that making them ES6 would only make things more complicated with little benefit to the end user. For example, since Node doesn't yet support ES6, we'd have to use babel-node in the build-unicode-data script (either that or transpile /tools to ES5, then run them with node, then transpile the files the end user uses). Additionally, since the files in /tools/output only have one export, I don't think that making them ES6 modules has much/any tree-shaking benefits for the end user.

---

Let me know if I've misunderstood anything or need to clarify something, thanks again!

EDIT: I came across a somewhat related issue about packaging XRegExp, if you're interested: #182

[brettz9]

Maybe you could take a look at master...brettz9:rollup ?

This preserves the status quo as far as the existing directory structure (though if you don't mind a breaking change, we could get rid of /XRegExp-all.js in favor of the new dist/index-umd.js). I left the test require() statements alone and just adding CommonJS plugins to the Rollup routine. So no need for browserify anymore.

However, for the sake of sanity in testing, I updated the current dependencies in this PR (and also added the package-lock.json), as I didn't want to have to build with the outdated dependencies and then retool things if the current versions behaved differently. (There are a couple small linting changes in the tests included as a result of the update to ESLint; I could separate these out but then the build would fail as a result of the new ESLint complaining.)

[brettz9]

Oh, and I can remove the dist files in .gitignore as you've done for XRegExp-all.js Personally I like including dist in the repo as it is helpful for sanity checking and also for deploying demos on Github-based sites like rawgit.com, but I can follow the same pattern as XRegExp-all.js if you like (of course they should be consistent)...

[josephfrazier]

This preserves the status quo as far as the existing directory structure (though if you don't mind a breaking change, we could get rid of /XRegExp-all.js in favor of the new dist/index-umd.js)

Thanks, let's keep the structure as-is for now.

However, for the sake of sanity in testing, I updated the current dependencies in this PR (and also added the package-lock.json), as I didn't want to have to build with the outdated dependencies and then retool things if the current versions behaved differently. (There are a couple small linting changes in the tests included as a result of the update to ESLint; I could separate these out but then the build would fail as a result of the new ESLint complaining.)

If you could make the dependency updates and package-lock.json changes separately, I'd appreciate it. That way, we can get those in regardless of the Rollup changes. The linting changes should be included in the PR that updates ESLint, so the build shouldn't fail.

Oh, and I can remove the dist files in .gitignore as you've done for XRegExp-all.js Personally I like including dist in the repo as it is helpful for sanity checking and also for deploying demos on Github-based sites like rawgit.com, but I can follow the same pattern as XRegExp-all.js if you like (of course they should be consistent)...

Yes, let's ignore the dist/ files, for the same reasons as ignoring XRegExp-all.js and lib/ (described in #182 (comment))

[brettz9]

FWIW, besides making a new PR for the package-lock.json addition, devDeps updates, and related linting fixes, I've rebased my rollup branch onto that PR (and plan to keep it in sync).

[josephfrazier]

Perfect, thanks! Hopefully we can land that one soon

[josephfrazier]

Now that #241 is merged, if you could make a PR for your rollup branch, it'd be easier to review and keep track of.

[brettz9]

PR added at #243

[josephfrazier]

Thanks, I'll close this in favor of #243, where the discussion continues.