Rollup #243

brettz9 · 2018-04-26T03:01:22Z

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 · 2018-04-26T03:01:29Z

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 · 2018-04-26T03:06:27Z

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?

josephfrazier

This looks really good for the most part!

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)?

That might be a good step to take later, but I think it's fine to leave things as-is for now.

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?

The build script only happens on local installs (that is, running npm install in a checkout of the repo). npm install xregexp does not run the build script.

I added a couple inline questions/comments as well.

josephfrazier · 2018-05-06T19:34:41Z

rollup.config.js

+                    'array-includes',
+                    'transform-xregexp'
+                ]
+            }),


Is there a way to reduce the duplication between these babel options and the .babelrc file? Perhaps rollup could generate the files in lib/ instead of the npm run babel script that's part of npm run prebuild?

Re: one aspect of .babelrc duplication, I could probably just import the .babelrc file here and adapt it...

However, as far as Rollup replacing the script acting on lib, no, I don't think so. Rollup is concerned with rolling up the files into a single file whereas Babel just provides the backward-compatible code (and which Rollup itself uses as a dependency).

That sounds good. It looks like we should just move the contents of .babelrc into the babel property of package.json. That way, we can import/require it easily from rollup.config.js.

https://babeljs.io/docs/usage/babelrc

Yes, that should work, too...

josephfrazier · 2018-05-06T19:35:36Z

package.json

@@ -16,18 +16,21 @@
    "unicode"
  ],
  "main": "./lib",
+  "module": "./src/index.js",


I think we'd probably want the module file to point to something that has passed through babel, to make sure that the only non-ES5 feature used is the import/export syntax.

EDIT: See https://github.com/rollup/rollup/wiki/pkg.module#wait-it-just-means-import-and-export--not-other-future-javascript-features

Ok, sure. I had got it into my head that Rollup took advantage of having access to the source for optimizations, but I guess I was mistaken.

…cemaps) - Enhancement: Add `module` to `package.json` - npm: Remove browserify in favor of Rollup

brettz9 · 2018-05-12T00:52:26Z

I've updated the branch.

josephfrazier · 2018-05-20T15:04:48Z

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 · 2018-05-20T22:16:11Z

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).

…exibly

josephfrazier · 2018-05-20T23:20:41Z

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 f6e32c6) to 244037 bytes. Are you seeing the same thing?

…produces a better-minified file) - Linting: Though quotes not enforced, be consistent within page in using single quotes

brettz9 · 2018-05-21T02:31:32Z

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

This makes it so that only `dist/index-es.js` is added to the files published, and so that the package size (shown by `npm pack`) is 220 kB rather than 493 kB, compared to the master branch size of 158 kB. Since we didn't publish minified files before, we don't necessarily need to start now, especially since users should be minifying for themselves.

josephfrazier · 2018-05-21T19:37:07Z

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 · 2018-05-26T00:58:27Z

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 · 2020-02-12T17:50:31Z

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 · 2020-02-13T00:52:00Z

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 · 2020-02-13T01:03:50Z

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 · 2020-02-13T01:15:56Z

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 · 2020-02-13T01:19:28Z

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 · 2020-02-13T01:20:05Z

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 · 2020-02-13T01:22:23Z

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 · 2020-02-13T01:28:01Z

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 · 2020-02-13T01:31:46Z

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

josephfrazier · 2021-01-09T06:01:02Z

Hey @brettz9 and @jsg2021, I'm starting to look back into using rollup. I've tried to update both this PR and #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 · 2021-01-09T06:14:51Z

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 · 2021-01-09T06:16:39Z

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

brettz9 · 2021-01-09T06:43:21Z

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 · 2021-02-06T20:39:41Z

@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 · 2021-02-07T18:24:04Z

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.

brettz9 mentioned this pull request Apr 28, 2018

Add module to package.json #234

Closed

brettz9 force-pushed the rollup branch from ba19981 to a1e39ed Compare May 3, 2018 00:07

josephfrazier reviewed May 6, 2018

View reviewed changes

brettz9 force-pushed the rollup branch 4 times, most recently from a1aec53 to 14adef9 Compare May 12, 2018 00:46

- Enhancement: Add Rollup ES distribution (with minification and sour…

6996368

…cemaps) - Enhancement: Add `module` to `package.json` - npm: Remove browserify in favor of Rollup

brettz9 force-pushed the rollup branch from 14adef9 to 6996368 Compare May 12, 2018 00:51

- Avoid add-module-exports for Rollup build

56eb1bc

josephfrazier added 2 commits May 20, 2018 18:27

Ignore add-module-exports for Rollup more explicitly/flexibly

97fd120

Set babel-preset-env modules to false for Rollup more explicitly/fl…

b91292c

…exibly

- Add plug-in "external-helpers" recommended by rollup-plugin-babel (…

df38ac8

…produces a better-minified file) - Linting: Though quotes not enforced, be consistent within page in using single quotes

josephfrazier added 2 commits May 21, 2018 15:30

Don't use rollup-plugin-re, rollup-plugin-commonjs makes it unnecessary

2b903b1

jsg2021 mentioned this pull request Jan 7, 2019

Release v4.2.3 depends on @babel/runtime == 7.0.0-beta.53 #264

Closed

jsg2021 mentioned this pull request Jan 31, 2020

Use rollup to simplify build/babel/lint #282

Closed

josephfrazier mentioned this pull request Feb 13, 2020

Add module to package.json for rollup/webpack #281

Merged

josephfrazier self-requested a review February 19, 2020 23:48

Merge branch 'master' into 243rollup

998c131

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rollup #243

Rollup #243

brettz9 commented Apr 26, 2018

brettz9 commented Apr 26, 2018 •

edited

brettz9 commented Apr 26, 2018

josephfrazier left a comment

josephfrazier May 6, 2018

brettz9 May 6, 2018

josephfrazier May 7, 2018

brettz9 May 7, 2018

josephfrazier May 6, 2018 •

edited

brettz9 May 6, 2018

brettz9 commented May 12, 2018

josephfrazier commented May 20, 2018 •

edited

brettz9 commented May 20, 2018 •

edited

josephfrazier commented May 20, 2018

brettz9 commented May 21, 2018 •

edited

josephfrazier commented May 21, 2018

brettz9 commented May 26, 2018

jsg2021 commented Feb 12, 2020

brettz9 commented Feb 13, 2020

jsg2021 commented Feb 13, 2020

brettz9 commented Feb 13, 2020

brettz9 commented Feb 13, 2020

jsg2021 commented Feb 13, 2020

brettz9 commented Feb 13, 2020 •

edited

jsg2021 commented Feb 13, 2020

brettz9 commented Feb 13, 2020

josephfrazier commented Jan 9, 2021 •

edited

jsg2021 commented Jan 9, 2021

jsg2021 commented Jan 9, 2021

brettz9 commented Jan 9, 2021

slevithan commented Feb 6, 2021

josephfrazier commented Feb 7, 2021 •

edited

Rollup #243

Are you sure you want to change the base?

Rollup #243

Conversation

brettz9 commented Apr 26, 2018

brettz9 commented Apr 26, 2018 • edited

brettz9 commented Apr 26, 2018

josephfrazier left a comment

Choose a reason for hiding this comment

josephfrazier May 6, 2018

Choose a reason for hiding this comment

brettz9 May 6, 2018

Choose a reason for hiding this comment

josephfrazier May 7, 2018

Choose a reason for hiding this comment

brettz9 May 7, 2018

Choose a reason for hiding this comment

josephfrazier May 6, 2018 • edited

Choose a reason for hiding this comment

brettz9 May 6, 2018

Choose a reason for hiding this comment

brettz9 commented May 12, 2018

josephfrazier commented May 20, 2018 • edited

brettz9 commented May 20, 2018 • edited

josephfrazier commented May 20, 2018

brettz9 commented May 21, 2018 • edited

josephfrazier commented May 21, 2018

brettz9 commented May 26, 2018

jsg2021 commented Feb 12, 2020

brettz9 commented Feb 13, 2020

jsg2021 commented Feb 13, 2020

brettz9 commented Feb 13, 2020

brettz9 commented Feb 13, 2020

jsg2021 commented Feb 13, 2020

brettz9 commented Feb 13, 2020 • edited

jsg2021 commented Feb 13, 2020

brettz9 commented Feb 13, 2020

josephfrazier commented Jan 9, 2021 • edited

jsg2021 commented Jan 9, 2021

jsg2021 commented Jan 9, 2021

brettz9 commented Jan 9, 2021

slevithan commented Feb 6, 2021

josephfrazier commented Feb 7, 2021 • edited

brettz9 commented Apr 26, 2018 •

edited

josephfrazier May 6, 2018 •

edited

josephfrazier commented May 20, 2018 •

edited

brettz9 commented May 20, 2018 •

edited

brettz9 commented May 21, 2018 •

edited

brettz9 commented Feb 13, 2020 •

edited

josephfrazier commented Jan 9, 2021 •

edited

josephfrazier commented Feb 7, 2021 •

edited