mishoo / UglifyJS

JavaScript parser / mangler / compressor / beautifier toolkit
http://lisperator.net/uglifyjs/
Other
13.19k stars 1.25k forks source link

require.resolve #5764

Open mbrevda opened 1 year ago

mbrevda commented 1 year ago

This project relies heavily on require.resolve. While this was initially introduced for better webpack support, this causes issue with other bundlers that don't support require.resolve such as esbuild.

I was unable to determine why the project was initially set up this way. Is there any history that can be pointed to? Understanding the use case can be helpful in figuring out a way forward for wider bundler support.

Thanks!

alexlamsl commented 1 year ago

Simply put, the code base is monolithic − source files are separated the same way the Web used to parse them:

<script src="parse.js"></script>
<script src="compress.js"></script>
<script src="minify.js"></script>
<!-- etc. -->

Conversion to a modular approach would incur a lot of work, both in development hours and more importantly require() overhead, without contributing to readability or ease of maintenance.

The preferred way of inclusion (which I guess many third party dependents of uglify-js chose to ignore) is to run:

$ bin/uglifyjs --self [--mangle --compress] --output path/to/deploy/uglify.js

since the result would be free of require() / require.resolve().

mbrevda commented 1 year ago

Not sure I follow (every other package manages just fine with require()/import), although the solution seems reasonable.

Would it be feasible to distribute an official build via npm? This will hopefully make it easier for consuming libs to adopt making it simpler for end users to build their code as they please.

alexlamsl commented 1 year ago

AFAICT this project started off before require() existed, so no questioning hysterical raisins here :smirk:

Just importing (& maintaining) all the AST_* classes alone would be a major headache.

But your suggestion to distribute (& in fact, run) the packaged version on npm seems like a good idea to bridge both worlds − if you are interested please consider making a Pull Request.

Alternatively, I can stick it to the end of my TODO list and may eventually get around to take a look at it.

mbrevda commented 1 year ago

AFAICT this project started off before require() existed, so no questioning hysterical raisins here 😏

That would explain it, thanks!

But your suggestion to distribute (& in fact, run) the packaged version on npm seems like a good idea to bridge both worlds − if you are interested please consider making a Pull Request.

I'd give it a try. Do you think just the built version should be distributed, or both built and unbuilt code should be included?

edit: the benefit of including just the built version would mean that all consuming lib's would use this version by default when they next update their deps. This can also be achieved by pointing package.json's main field at the built version when distributing both.

alexlamsl commented 1 year ago

This can also be achieved by pointing package.json's main field at the built version when distributing both.

Yes, I think if we put the packaged file as /index.js and update package.json accordingly it should just work.

If we want to maximise compatibility we can also include the existing /lib/*, though I'm slightly inclined to omit for a minor-version release because:

mbrevda commented 1 year ago

Is there a quick way (a la --self) to build the bin?

mbrevda commented 1 year ago

Is there a quick way (a la --self) to build the bin?

nm, I think I got it - see linked PR

mattcasey commented 1 year ago

I think I'm running into this issue. We are using serverless to deploy a Lambda function that uses mjml for email templates. mjml imports uglify-js as a production dependency, but esbuild cannot follow require.resolves so it blows up.

shellscape commented 9 months ago

Ran into the same issue as @mattcasey exception with https://jsx.email. It uses rehype-minify, which uses uglify. Uglify doesn't play well with lambdas.

zupmeL commented 5 months ago

+1