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

[WIP] upgrade Math.js to v6.6.0 and Rollup-based build #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

leeoniya
Copy link

@leeoniya leeoniya commented Feb 8, 2020

hey @josdejong , this is for #3.

with this PR, running rollup -c produces these warnings:

./index.js → ./dist/mathjs-expression-parser.js...
(!) `this` has been rewritten to `undefined`
https://rollupjs.org/guide/en/#error-this-is-undefined
node_modules\typed-function\typed-function.js
23:     root.typed = factory();
24:   }
25: }(this, function () {
      ^
26:
27:   function ok () {
(!) Plugin rollup-plugin-cjs-es: node_modules\mathjs\es\plain\number\relational.js is not loaded.
created ./dist/mathjs-expression-parser.js in 2.9s
  1. to fix typed-function.js i removed its umd wrapper & made it export default create(); at the bottom.
  2. relational.js is an empty file, so that warning can probably be ignored.

the resulting minified file is quite large - 122KB (certainly not 30 KiB!). without diving into the actual file yet, i think this in part due to many transient commonjs dependencies - many with UMD wrappers that cannot be dead-code-eliminated (tree-shaken, if you will). it might be worth trying to reduce these deps by either switching to ES alternatives or just absorbing the [often small] deps into some local /libs and tweaking them for ES exports.

perhaps you can take a look at the non-minified dist build and spot anything obvious/large that should not be included in the parser-only bits.

@leeoniya
Copy link
Author

leeoniya commented Feb 8, 2020

something else to consider is to make Math.js depend on mathjs-expression-parser rather than the other way around. this would reduce the risk of accidentally pulling in unwanted bits into the parser.

@josdejong
Copy link
Owner

Thanks for all your work Leon! I tried out your PR, and it bundles fine, but I can't run both examples (node.js and browser), is that still work in progress?

the resulting minified file is quite large - 122KB (certainly not 30 KiB!).

This 30 KiB is the minified and gzipped size. So if you compress it in a tar.gz file it's about 30KiB. So this zipped size of 122KB is to be expected. So the dead code elimination works, else the file would be way bigger :).

The typed-function library is indeed also still an old-fashioned UMD library (the source is currently simply one file with UMD wrapper around the source). It would be nice to address that too, but that's a separate topic (unless it's really blocking for this issue).

@leeoniya
Copy link
Author

is that still work in progress?

honestly, i was hoping that i was doing something wrong, because 122KB is much larger than i was looking for. i thought it would be 30k minified and i could shave 50-60% via Rollup & DCE, but from looking at the bundle code, there'd have to be extensive changes to get the size down any significant amount :(

i found this [1] which may serve my purposes better, is 0-dep and 2k minified. i might need to add function/param handling to it, but that seems do-able.

feel free to take over and finish up this PR.

thanks for your help!

[1] https://github.com/gumm/inpostfix

@josdejong
Copy link
Owner

Sounds good. Yes, you do have more lightweight parsers. For example https://github.com/bugwheels94/math-expression-evaluator is very neat too.

Will have a look in the future. It may also be that this library is not needed that much since mathjs supports tree shaking now and it's easier to cherry pick what you need.

@skosito
Copy link

skosito commented Jul 18, 2020

Hey @josdejong @leeoniya how we can only import parser (or math.parse function) and make sure tree shaking removes as much code as possible? I tried out to import 'create' and 'parse' only but that is importing basically whole lib again... Since I only want expression parser, do you recommend using math-expression-evaluator instead, and are there some missing functionalities there compared to mathjs parser? Thanks!

@josdejong
Copy link
Owner

@skosito the following page describes how it works and what to expect from tree shaking, see bottom of the page: https://mathjs.org/docs/custom_bundling.html

The expression parser only is still quite large. If you need something more lightweight you can indeed use math-expression-evaluator for example.

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