Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Rollup bundle #1533

Merged
merged 2 commits into from
Apr 26, 2021
Merged

Rollup bundle #1533

merged 2 commits into from
Apr 26, 2021

Conversation

realityking
Copy link
Contributor

What issue does this pull request resolve?

As mentioned in #1526 (comment), this changed the build process for the bundles to create source maps that are based of the original Typescript files. As an added advantage, they're also much smaller!

I'm including again the change not to ship source maps as it's now unambiguous that the source maps are not needed for the browser bundles.

What changes did you make?

  • Replace browserify with rollup
  • Remove scripts/bundle.js with a rollup config file
  • Builds are directly from ts files, so npm run bundle can be run without first running npm run build
  • Disabled generation of source maps for the dist build to save the size in the npm package. (see Smaller package #1526 for prior discussion)

Is there anything that requires more attention while reviewing?

Overview of size changes:

File master rollup change
ajv2019.bundle.js 280K 268K -4.3%
ajv2019.min.js 136K 108K -20.6%
ajv2019.min.js.map 180K 208K +15.5%
ajv7.bundle.js 264K 252K -4.5%
ajv7.min.js 124K 100K -19.4%
ajv7.min.js.map 168K 200K +19%
ajvJTD.bundle.js 256K 240K -6.25%
ajvJTD.min.js 120K 96K -20%
ajvJTD.min.js.map 168K 200K +19%

The source maps are quite a bit bigger but the actual code files are a lot smaller.

@realityking realityking force-pushed the rollup-bundle branch 2 times, most recently from 02218fa to c4f08e9 Compare April 5, 2021 21:25
@epoberezkin
Copy link
Member

Thank you!

@realityking
Copy link
Contributor Author

I pushed a small fix. I had omitted removing the bundle folder before building bundles.

@epoberezkin
Copy link
Member

@realityking the main reason I didn't merge yet was that I was wondering whether there could be the contexts where source maps are needed?

See: #1532

Could we maybe do it in two steps:

  1. release rollup bundling (without removing source maps from npm)
  2. make a patch release that does only source map removal, so that if something is broken I can release the next patch reverting it.

In v9 I would also remove typescript source from npm, so that if somebody needs to recompile they would have to use GitHub, but it's relatively rare to justify having it all in npm... Currently there are dependencies on source.

@epoberezkin
Copy link
Member

Also, rollup has an added benefit of warning about circular dependencies that was quite an effort to remove, so I'd rather they don't creep back in... Is it maybe possible to fail in case of circular dependencies warnings?

epoberezkin
epoberezkin previously approved these changes Apr 24, 2021
@realityking
Copy link
Contributor Author

I rebased this PR and dropped source map commit.

Also, rollup has an added benefit of warning about circular dependencies that was quite an effort to remove, so I'd rather they don't creep back in... Is it maybe possible to fail in case of circular dependencies warnings?

I'm not sure what you're asking for. Do you want the build to fail if there's a circular dependency warning? Rollup has --failAfterWarnings but that will fail the build for any warning. If that not desired a custom script would have to be build to check the warnings on only fail for specific ones.

@epoberezkin
Copy link
Member

If that not desired a custom script would have to be build to check the warnings on only fail for specific ones.

I see... No problem, at least the warnings would be more visible.

@epoberezkin epoberezkin merged commit 00b0e24 into ajv-validator:master Apr 26, 2021
@realityking realityking deleted the rollup-bundle branch April 26, 2021 18:07
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants