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

For whatever reason eval being executed in production mode #818

Closed
solomax opened this issue May 20, 2022 · 16 comments
Closed

For whatever reason eval being executed in production mode #818

solomax opened this issue May 20, 2022 · 16 comments

Comments

@solomax
Copy link

solomax commented May 20, 2022

TL;DR;

for whatever reason this https://github.com/mathjax/MathJax-src/blob/master/ts/components/version.ts#L37 code being executed at my app :(

Long version:
We are using MathJax to draw beautiful formulas at Apache OpenMeetings: https://github.com/apache/openmeetings/blob/master/openmeetings-web/src/main/front/wb/package.json#L22
And browserify as build tool

We are using some sort node-like configuration https://github.com/apache/openmeetings/blob/master/openmeetings-web/src/main/front/wb/src/wb-tool-stat-math.js#L4

Everything was working as expected (and still working in DEV mode)
BUT for whatever reason in PRODUCTION eval is being called to determine the MathJax version :(

I've hacked this by adding window.PACKAGE_VERSION = '3.2.1'; to my source code
But this a bit dirty :(

Maybe it would be possible to introduce some eval-free mode?
Or maybe I'm doing something wrong?
Or maybe it would be possible to store version in some pre/post build step? :)))

Thanks in advance for the help :)

@dpvc
Copy link
Member

dpvc commented May 20, 2022

MathJax uses the webpack DefinePlugin to replace PACKAGE_VERSION by the actual version:

const package = path.resolve(__dirname, '..', 'package.json');
//
// Record the js directory for the pack command
//
return [new webpack.DefinePlugin({
__JSDIR__: jsdir,
PACKAGE_VERSION: `'${require(package).version}'`
})];
};

(so web components will have the value) and tries to look it up at run time otherwise (for node applications). So if you are using some other package manage, you will need to do something similar. It looks like maybe the browserify-replace package might be able to do the job for you.

Sorry that this update has had this negative impact for you.

@solomax
Copy link
Author

solomax commented May 23, 2022

Thanks!
was able to properly work-around this :)

@solomax solomax closed this as completed May 23, 2022
@ryangr-texthelp
Copy link

ryangr-texthelp commented May 23, 2022

Just to chip in here, we have also run into this issue.

Unfortunately, for us, we are using mathjax-full as a dependency for our library so we are not bundling it directly. Consumers of our library may use a bundler, but it would be difficult for us to try and enforce this as part of a consumer build process.

I understand the need for PACKAGE_VERSION to be defined, and it's great that it happens as part of the build process, but equally it can be hardcoded in source and overwritten at runtime if required.

If we go down this route, we're going to have to have separate instructions for Rollup, Webpack, Browserify, and so on.... simply for reading the version number.

Suggestion to simply target Node.js environments by default, and fallback to a hardcoded string (can be generated at check-in/hooks if required?):

const PACKAGE_VERSION = "3.2.1";

export const VERSION = (
  typeof window === 'undefined' ?
    //
    //  This will not be included in the webpack version, so only runs in node
    //
    (function () {
      //
      //  Look up the version from the package.json file
      //
      /* tslint:disable-next-line:no-eval */
      const load = eval('require');
      /* tslint:disable-next-line:no-eval */
      const dirname = eval('__dirname');
      const path = load('path');
      return load(path.resolve(dirname, '..', '..', 'package.json')).version;
    })() :
  PACKAGE_VERSION
);

@ShaMan123
Copy link

ShaMan123 commented May 24, 2022

I haven't had time to dive deep into this but I agree with @ryangr-texthelp
I maintain a library that depends on mathjax as well.
(@solomax funny seeing you here)

@solomax
Copy link
Author

solomax commented May 24, 2022

@ShaMan123 :)
We are using both both fabric.js and MathJax
It was fun to make fabric to draw beautiful formulas :)

@ShaMan123
Copy link

It was fun to make fabric to draw beautiful formulas :)

Nice. I do it as well in a project.
BTW I see you use a whiteout functionality. Take a look at EraserBrush.

@NicolasCARPi
Copy link

Hello,

Could this issue be reopened please?

I fully agree with @ryangr-texthelp's message: the current approach to get a simple VERSION string is flawed and IMHO over-complicated. Now don't get me wrong, @dpvc had good reasons to do it like this I'm sure. But in the end many users spent way too much time trying to figure out the correct way to feed the PACKAGE_VERSION to the lib. I ended up overwriting the source code to hardcode it before bundling it.

I believe the issue is with projects building mathjax from source. For some reason I couldn't make the DefinePlugin approach work (I'm using webpack). As Ryan said, this won't work down the line for many users. Or even in 2 years when webpack gets replaced ;p

It would be great if this issue could be adressed in the next patch. :)

Best,
~Nico

@dpvc
Copy link
Member

dpvc commented May 30, 2022

[Sorry about the slow response, here. I've been away for a week, and am trying to catch up now.]

Although we were trying to make things easier, we clearly made things harder for some use-cases like yours, and need to re-evaluate the situation. I've made a feature request in the MathJax issue tracker, and will address it in the next release. There may well be another point release soon to address this plus another issue that has affected the lazy-typesetting extension.

@NicolasCARPi
Copy link

Hey @dpvc.

I'm glad to read that. Hopefully I can remove that ugly script of mine to hardcode the version soon :)

I also agree that a patch release is necessary. If this could be integrated, it would be great too!

Best,
~Nico

@solomax
Copy link
Author

solomax commented May 31, 2022

Thanks for addressing this @dpvc :))

@dpvc
Copy link
Member

dpvc commented Jun 2, 2022

I've made a pull request that should resolve this issue. It creates the version.ts file using the package.json version number during the travis-ci publish script, so the version will just be a constant, as requested.

@NicolasCARPi
Copy link

Great, thanks @dpvc !

edemaine added a commit to edemaine/tex2svg-webworker that referenced this issue Jun 2, 2022
MathJax waiting on bug fix: mathjax/MathJax-src#818
dpvc added a commit that referenced this issue Jun 8, 2022
Make version.ts use a constant and create the file during the build process.  (#818)
@solomax
Copy link
Author

solomax commented Jun 10, 2022

Just have updated to the latest release 3.2.2
Works as expected without hacks :)
Thanks for the fix!

@dpvc
Copy link
Member

dpvc commented Jun 10, 2022

@solomax, thanks for the confirmation. Glad it works better for you.

@NicolasCARPi
Copy link

Yes, can confirm here too 👍 Cheers!

@dpvc
Copy link
Member

dpvc commented Jun 11, 2022

@NicolasCARPi, great thanks!

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

No branches or pull requests

5 participants