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

Feature/migrate html minifier terser #378

Merged

Conversation

multivoltage
Copy link
Contributor

@multivoltage multivoltage commented Jul 6, 2023

Checklist

Goal

I saw an issue of an user that ask support for html-minifier-terser in place of html-minifier.
Only thing that change is that minify method is a promise.

Behavior using old html-minifier

Since in js we can do

function syncMethod(){
   return "hello";
}
const output = await syncMethod()

Even point-of-view will add the terser version which is Promise based, configuring options like this:

const minifier = require('html-minifier')
// optionally defined the html-minifier options
const minifierOpts = {
  removeComments: true,
}
  options: {
    useHtmlMinifier: minifier,
    htmlMinifierOptions: minifierOpts
  }

can be done also using html-minifier instead html-minifier-terser

@multivoltage multivoltage mentioned this pull request Jul 6, 2023
2 tasks
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm terrified of mixing callbacks and promises in this way. In my experience, it is the recipe for:

  1. performance issues
  2. memory leaks
  3. race conditions

I'm ok with moving the whole codebase to an async/await or just adding a .then() to the calls to the minifier. However the best approach would be to drop the callbacks and use async/await everywhere.

@multivoltage
Copy link
Contributor Author

In my small experience I prefer only await and async instead callback. But there are some case where we cannot wait a "line" but only start a "parallel" method.
I will do an analysis next day.

If we agree to replace all callback with await async, do you you think I can put changes in that pr ?

@mcollina
Copy link
Member

mcollina commented Jul 7, 2023

It's better to do the two things separately

@multivoltage
Copy link
Contributor Author

Ok. I will try to use callback for this or. Thanks

@multivoltage
Copy link
Contributor Author

@mcollina when you say to migrate all codebase to async await you are considering also for example readFile() or only calls "then()" for promises?

@mcollina
Copy link
Member

All of it, yes.

@multivoltage multivoltage force-pushed the feature/migrate-html-minifier-terser branch from ba6d662 to 9bc223e Compare February 19, 2024 18:10
@multivoltage
Copy link
Contributor Author

I resume this PR since async flow was introduced

@mcollina
Copy link
Member

CLosing for no activity

@mcollina mcollina closed this Feb 28, 2024
@multivoltage
Copy link
Contributor Author

OK.
I updated PR (like I said in the previous comment).
Did you expect other changes?

@gurgunday gurgunday reopened this Feb 28, 2024
Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina merged commit 8113f2e into fastify:master Feb 28, 2024
38 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.

None yet

3 participants