colbyfayock / html-webpack-partials-plugin

🛠 Easy HTML partials for Webpack without a custom index!
MIT License
68 stars 20 forks source link

Fix get images #51

Open sgb004 opened 2 years ago

sgb004 commented 2 years ago

I was looking for a plugin that could easily reuse an HTML fragment in webpack and I was very pleasantly surprised to see that HTMLWebpackPartialsPlugin could do what I was looking for and in such a simple and easy to use way.

However, I found a small problem, the plugin did not process the imgs tags, when I searched for an answer I found that some people also had the same problem https://github.com/colbyfayock/html-webpack-partials-plugin/issues/7 So I tried to take a look at the code, after reviewing the plugin code, I came to the conclusion that it did not have a way to process the imgs tags so that later webpack could extract the images, I imagine that initially webpack, HTMLWebpackPlugin or html-loader were getting the images, but I guess with the latest updates this was no longer the case. So I tried to correct this little problem.

With this pull-request my intention is that the plugin can process the imgs tags and that webpack, HTMLWebpackPlugin or html-loader can process the images. So that the plugin could process the imgs tags, what I did was create a new instance of HTMLWebpackPlugin and let HTMLWebpackPlugin process the partial, then through the processAssets hook with the stage PROCESS_ASSETS_STAGE_SUMMARIZE obtain the html of the partial already processed to finally inject it into the template or templates.

I replaced the current PROCESS_ASSETS_STAGE_SUMMARIZE hook of HTMLWebpackPlugin with 3 hooks:

All of the above I modified in index.js

To achieve these changes I also modified partial.js, where I added a unique_name with which to identify the file and thus be able to obtain the html and later delete it. In this file I also modified the createTemplate method where it now receives the html and converts it to a loash template instead of getting it directly from the source file like it currently is.

It seems to me that this is a crude way of processing the partial, I made another version where I managed to obtain the code that processes the file in HTMLWebpackPlugin, this with the aim of trying to improve performance and not load the entire core of HTMLWebpackPlugin, however in the tests the times remained almost the same. You can check the alt version in https://github.com/sgb004/html-webpack-partials-plugin/tree/fix_get_imgs_alt_ver

Other changes I made was updating the dependencies, I added uuid and changed the mocha processing time, I had to update the tests since webpack no longer returns the source, I changed the code of the tests so that instead of getting result.compilation.assets[‘file.html'].source() the html is obtained using fs. Updating webpack and HTMLWebpackPlugin caused the html of the examples to change, so I had to change the html of test/fixtures, really the only change I made was to move the script from the body to the header since HTMLWebpackPlugin puts it in the header, I wanted to keep the test/fixtures files as they were, but that would involve changing the settings in each of the examples and I preferred to keep the examples simple.

colbyfayock commented 2 years ago

thanks so much for putting this together! it'll take some time for me to review (and as you could tell time to get to it in the first place) but definitely interested in these changes

it looks like the tests failed due to the node version: https://github.com/colbyfayock/html-webpack-partials-plugin/runs/5694847186?check_suite_focus=true

i currently have the nvmrc pointed at v10, where im not opposed to bumping it up: https://github.com/colbyfayock/html-webpack-partials-plugin/blob/master/.nvmrc

it's been a while since i've worked directly in webpack, do you have any thoughts on changing the minimum required node version for this package? looks like it needs to be >12.13.0

would you mind bumping the version in .nvmrc based on that?

sgb004 commented 2 years ago

Hi!

Yes, of course.

sgb004 commented 2 years ago

I changed the version in .nvmrc and I changed the version of nodejs used in the workflow test, I think it was not necessary change the version in .nvmrc, with act https://github.com/nektos/act the test passed with nodejs 10 or 12 in .nvmrc but "act" failed when in /.github/workflows/tests.yml nodejs had 10 version.

Also, I made a change in a variable that I had added previously and which caused an error in the mocha tests and I added an timeout in the Analytics test.

colbyfayock commented 1 year ago

hey @sgb004 sorry that i'm only now responding to this. this project has taken a pretty low priority for me

that said I'm not comfortable with the changeset here. especailly given the amount of time im spending on this, i need to be extra cautious that something introduced isn't going to create issues for existing users of the plugin

seeing the output differences don't make me confident that things aren't broken, particularly:

that said i'm not really sure what the path for this looks like moving forward if you're still interested in working on this after all this time. i appreciate the contribution and apologize for how long it took to get back