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

Run Eval removing codeshift #1171

Merged

Conversation

sgoo
Copy link
Contributor

@sgoo sgoo commented Apr 8, 2022

Remove all the places where code is built as a string and then evaled to make a function. This improves compatibility with runtimes where eval is banned.

The codeshift does the minimal set of changes to make this work, and I have manually fixed a couple of things after running the codeshift.

  • Fix the DEFNODE function for the new param order.
  • Remove the map/mkshallow functions.

The source for the codeshift is here: https://github.com/sgoo/ast-fixer

I did have a go at replacing the calls to DEFNODE with class declarations, I got the code to run, but I got test failures where the minified output was slightly wrong, I'm not sure what caused that.
My results from that attempt are here: sgoo/ast-fixer@f77472e

Fixes #925

Remove all the places where code is built as a string and then evaled to
make a function. This improves compatability with runtimes where eval is
banned.
The codeshift does the minial set of changes to make this work, and I
have manually fixed a couple of things after running the codeshift.
* Fix the DEFNODE function for the new param order.
* Remove the map/mkshallow functions.

Fixes terser#925
@fabiosantoscode
Copy link
Collaborator

fabiosantoscode commented Apr 8, 2022

This is awesome and appreciated!

Looks good to merge from a cursory look, I'll review a bit later.

@Conduitry
Copy link
Contributor

Conduitry commented Apr 8, 2022

Should there be some sort of build process for generating all of this code at build time (rather than at runtime)? I assume one of the reasons for automating this is that hand-editing this code would be tedious and error-prone.

@fabiosantoscode
Copy link
Collaborator

fabiosantoscode commented Apr 8, 2022

@Conduitry I also thought about this. I'm not sure if I want to have a compilation step, especially now that we can run most tests without pre-compilation step, and use the node debugger really easily.

The only compilation that's really necessary is for running ./bin/terser, and, of course, releasing the project with the dist/bundle file in it.

@fabiosantoscode
Copy link
Collaborator

fabiosantoscode commented Apr 8, 2022

@sgoo I don't want to be the "you missed that spot" guy, and I'll do this if you don't, but there's still a (0, eval) call in evaluate.js. It's used to evaluate regexps, and is kind of unnecessary.

@fabiosantoscode
Copy link
Collaborator

fabiosantoscode commented Apr 9, 2022

There the eval goes :) 52da6d2

@fabiosantoscode
Copy link
Collaborator

fabiosantoscode commented Apr 9, 2022

After a second review, this looks good to merge. Thanks again for your effort @sgoo!

@fabiosantoscode fabiosantoscode merged commit a8ab684 into terser:master Apr 9, 2022
8 checks passed
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