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

Rollup #243

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Rollup #243

wants to merge 8 commits into from

Conversation

brettz9
Copy link
Contributor

@brettz9 brettz9 commented Apr 26, 2018

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

brettz9 commented Apr 26, 2018

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

brettz9 commented Apr 26, 2018

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?

Copy link
Collaborator

@josephfrazier josephfrazier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

rollup.config.js Outdated
'array-includes',
'transform-xregexp'
]
}),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that should work, too...

package.json Outdated
@@ -16,18 +16,21 @@
"unicode"
],
"main": "./lib",
"module": "./src/index.js",
Copy link
Collaborator

@josephfrazier josephfrazier May 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

- Enhancement: Add `module` to `package.json`
- npm: Remove browserify in favor of Rollup
@brettz9
Copy link
Contributor Author

brettz9 commented May 12, 2018

I've updated the branch.

@josephfrazier
Copy link
Collaborator

josephfrazier commented May 20, 2018

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

brettz9 commented May 20, 2018

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

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

brettz9 commented May 21, 2018

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

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

brettz9 commented May 26, 2018

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

jsg2021 commented Feb 12, 2020

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

brettz9 commented Feb 13, 2020

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

jsg2021 commented Feb 13, 2020

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

brettz9 commented Feb 13, 2020

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

brettz9 commented Feb 13, 2020

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

jsg2021 commented Feb 13, 2020

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

brettz9 commented Feb 13, 2020

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

jsg2021 commented Feb 13, 2020

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

brettz9 commented Feb 13, 2020

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

@josephfrazier josephfrazier self-requested a review February 19, 2020 23:48
@josephfrazier
Copy link
Collaborator

josephfrazier commented Jan 9, 2021

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

jsg2021 commented Jan 9, 2021

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

jsg2021 commented Jan 9, 2021

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

@brettz9
Copy link
Contributor Author

brettz9 commented Jan 9, 2021

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

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

josephfrazier commented Feb 7, 2021

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.

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.

4 participants