-
-
Notifications
You must be signed in to change notification settings - Fork 277
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
base: master
Are you sure you want to change the base?
Rollup #243
Conversation
Btw, before tackling documentation, should |
Also, I added |
There was a problem hiding this 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 indist
(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' | ||
] | ||
}), |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
a1aec53
to
14adef9
Compare
…cemaps) - Enhancement: Add `module` to `package.json` - npm: Remove browserify in favor of Rollup
I've updated the branch. |
Thanks, it looks like the changes so far leave However, there's one problem that I noticed with 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! |
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). |
Oh, that explains it, Babel's Hmm, wait: It looks like this actually increases the size (measured with |
…produces a better-minified file) - Linting: Though quotes not enforced, be consistent within page in using single quotes
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.
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. |
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 |
I've submitted an alternate to this PR (#282) In that PR, we use rollup to generate the |
That would work for me, but the reason I took the approach of a separate |
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. |
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 |
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 |
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. |
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. |
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. |
Ok, I see, my apologies then. Yes, that sounds good to me. |
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 EDIT: I'm currently getting this error when I run
|
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. |
But I don't really have a preference. Not having any dependencies on babel or core-js would be my only real desire. |
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 The PR should probably switch to the currently maintained |
@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. |
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. |
module
topackage.json
As discussed in #234