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

Add module to package.json #234

Closed
wants to merge 1 commit into from
Closed

Conversation

brettz9
Copy link
Contributor

@brettz9 brettz9 commented Apr 12, 2018

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
Copy link
Contributor Author

brettz9 commented Apr 12, 2018

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
Copy link
Owner

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
Copy link
Contributor Author

brettz9 commented Apr 16, 2018

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
Copy link
Contributor Author

brettz9 commented Apr 17, 2018

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
Copy link
Collaborator

josephfrazier commented Apr 17, 2018

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
Copy link
Contributor Author

brettz9 commented Apr 17, 2018

@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
Copy link
Contributor Author

brettz9 commented Apr 18, 2018

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
Copy link
Contributor Author

brettz9 commented Apr 18, 2018

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
Copy link
Collaborator

josephfrazier commented Apr 18, 2018

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
Copy link
Contributor Author

brettz9 commented Apr 23, 2018

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
Copy link
Contributor Author

brettz9 commented Apr 23, 2018

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
Copy link
Collaborator

josephfrazier commented Apr 23, 2018

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
Copy link
Contributor Author

brettz9 commented Apr 26, 2018

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
Copy link
Collaborator

Perfect, thanks! Hopefully we can land that one soon

@josephfrazier
Copy link
Collaborator

josephfrazier commented Apr 26, 2018

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 brettz9 mentioned this pull request Apr 26, 2018
@brettz9
Copy link
Contributor Author

brettz9 commented Apr 28, 2018

PR added at #243

@josephfrazier
Copy link
Collaborator

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

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.

3 participants