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

Introduce a minify-html plugin #1467

Closed
wants to merge 1 commit into from

Conversation

BlindDespair
Copy link
Contributor

@BlindDespair BlindDespair commented Nov 18, 2021

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@SanderElias I suggest to include MinifyHtml plugin as official @scullyio plugin since it is a useful plugin for the Scully ecosystem but seems to be no longer maintained while having certain issues.

I submit a PR that includes the plugin source code, docs for using it and build configs.
Besides that I updated it to use postProcessByHtml plugin type instead of deprecated render so Scully no longer gives a warning when running it with this plugin. (Related to original issue)
Also I fixed the bug where this plugin was messing with the transfer state script, so adding {minifyJS: true} is no longer required for Scully to work properly with this plugin.

I tested it locally with one of my projects and it worked with a small note, that when I used it via npm link Scully complained that plugin minifyHtml was not found, so I had to add registerPlugin manually in my project's config, but I believe it's due to how npm link works, plugin was registering itself in different instance of scully installation than what my project used.

@SanderElias
Copy link
Contributor

@BlindDespair, sorry that this is taking some time, but there are a couple of things slightly off, and that is our fault.
We are currently working on a schematic to add a new plugin to our repo, so that becomes much easier.
I'll ping you when we have that done. It's going to be a small refactor of your work, but in its current form, we can't merge it.

@BlindDespair
Copy link
Contributor Author

@SanderElias it's not a problem. I will fix it once your schematic is out.

@SanderElias
Copy link
Contributor

@BlindDespair, Ok, I will close this PR for now. and let you know when we are ready for this.

@SanderElias SanderElias closed this Dec 6, 2021
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.

2 participants