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

require.resolve #5764

Open
mbrevda opened this issue Dec 18, 2022 · 9 comments · May be fixed by #5765
Open

require.resolve #5764

mbrevda opened this issue Dec 18, 2022 · 9 comments · May be fixed by #5765

Comments

@mbrevda
Copy link

mbrevda commented Dec 18, 2022

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
Copy link
Collaborator

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
Copy link
Author

mbrevda commented Dec 19, 2022

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
Copy link
Collaborator

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

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
Copy link
Author

mbrevda commented Dec 19, 2022

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
Copy link
Collaborator

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:

  • you can't really require() any of those .js as is anyway, so chances of direct third-party dependence is slim
  • if it does happen, we can quickly include them back in for a .1 release

@mbrevda
Copy link
Author

mbrevda commented Dec 19, 2022

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

@mbrevda mbrevda linked a pull request Dec 19, 2022 that will close this issue
@mbrevda
Copy link
Author

mbrevda commented Dec 19, 2022

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

nm, I think I got it - see linked PR

@mattcasey
Copy link

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
Copy link

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.

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 a pull request may close this issue.

4 participants