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

Fix get images #51

Open
wants to merge 54 commits into
base: master
Choose a base branch
from
Open

Fix get images #51

wants to merge 54 commits into from

Conversation

sgb004
Copy link

@sgb004 sgb004 commented Mar 14, 2022

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 #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:

  • The first is beforeEmit from the HTMLWebpackPlugin, which returns a list of all files processed by HTMLWebpackPlugin.
  • The second is processAssets with the stage PROCESS_ASSETS_STAGE_SUMMARIZE, previously mentioned, with this the htmls of the partials and HTMLWebpackPlugin templates are obtained, in this hook the partials are injected into the templates.
  • And the third is processAssets with the stage PROCESS_ASSETS_STAGE_ANALYSE to delete the generated partials and prevent them from remaining in the distribution directory.

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.

sgb004 added 30 commits March 8, 2022 21:40
…esult.compilation.assets['index.html'].source() no longer works
…ipts in the end of body, instead it puts the script in the end of head.

I added the attr defer in the script tag because HTMLWebpackPlugin add that in the script tag now.
I think that these changes do not affect the expected result .
…by webpack because result.compilation.assets['index.html'].source() and result.compilation.assets['other-page.html'].source() no longer works
…TMLWebpackPlugin no longer puts scripts in the end of body, instead it puts the script in the end of head.

I added the attr defer in the script tag because HTMLWebpackPlugin add that in the script tag now.
I think that these changes do not affect the expected result.
…esult.compilation.assets['index.html'].source() no longer works
… in the end of body, instead it puts the script in the end of head.

I added the attr defer in the script tag because HTMLWebpackPlugin add that in the script tag now.
I think that these changes do not affect the expected result.
…result.compilation.assets['index.html'].source() no longer works
…s scripts in the end of body, instead it puts the script in the end of head.

I added the attr defer in the script tag because HTMLWebpackPlugin add that in the script tag now.
I think that these changes do not affect the expected result.
…result.compilation.assets['index.html'].source() and result.compilation.assets['other-page.html'].source() no longer works
…template-filename-other.html because HTMLWebpackPlugin no longer puts scripts in the end of body, instead it puts the script in the end of head.

I added the attr defer in the script tag because HTMLWebpackPlugin add that in the script tag now.
I think that these changes do not affect the expected result.
…result.compilation.assets['index.html'].source() no longer works
…o longer puts scripts in the end of body, instead it puts the script in the end of head.

I added the attr defer in the script tag because HTMLWebpackPlugin add that in the script tag now.
I think that these changes do not affect the expected result.
…result.compilation.assets['index.html'].source() no longer works
… puts scripts in the end of body, instead it puts the script in the end of head.

I added the attr defer in the script tag because HTMLWebpackPlugin add that in the script tag now.
I think that these changes do not affect the expected result.
…pplied to the plugin and to adapt to the latest version of webpack and HTMLWebpack
…result.compilation.assets['index.html'].source() no longer works
…s in the end of body, instead it puts the script in the end of head.

I added the attr defer in the script tag because HTMLWebpackPlugin add that in the script tag now.
And also how I changed the documentation of the example.
…result.compilation.assets['index.html'].source() and result.compilation.assets['about.html'].source() no longer works
…se HTMLWebpackPlugin no longer puts scripts in the end of body, instead it puts the script in the end of head.

I added the attr defer in the script tag because HTMLWebpackPlugin add that in the script tag now.
I think that these changes do not affect the expected result.
…result.compilation.assets['index.html'].source() and result.compilation.assets['about.html'].source() no longer works
…ripts in the end of body, instead it puts the script in the end of head.\nI added the attr defer in the script tag because HTMLWebpackPlugin add that in the script tag now.\nI think that these changes do not affect the expected result.
…result.compilation.assets['index.html'].source() and result.compilation.assets['about.html'].source() no longer works
…ts scripts in the end of body, instead it puts the script in the end of head.\nI added the attr defer in the script tag because HTMLWebpackPlugin add that in the script tag now.\nI think that these changes do not affect the expected result.
…result.compilation.assets['index.html'].source() and result.compilation.assets['about.html'].source() no longer works
…result.compilation.assets['index.html'].source() and result.compilation.assets['about.html'].source() no longer works
…ipts in the end of body, instead it puts the script in the end of head.

I added the attr defer in the script tag because HTMLWebpackPlugin add that in the script tag now.
I think that these changes do not affect the expected result.
The time was increased in the tests due to the fact that sometimes mocha gives false consumption of time.
@colbyfayock
Copy link
Owner

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

sgb004 commented Mar 30, 2022

Hi!

Yes, of course.

It didn't really need to be static.
The reason for the change was because it was causing an error in the Page Template test, I will try to explain how it caused that error:
The filesProcessed variable saves the name of the files processed by HTMLWebpackPlugin and is used later to inject the partial, this occurs when in the configurations template_filename is equal to *, because the variable was static it was causing an error in the Page test Template, when mocha went through that test, the filesProcessed variable had saved the name of the files processed by HTMLWebpackPlugin from the tests prior to this test, when doing the iteration of the variable to obtain each file processed by HTMLWebpackPlugin and when trying to obtain the file, it could not be found because it belonged to another test causing the Page Template test to fail.
…e Page Template test, probably due to changes I made to the plugin.
@sgb004
Copy link
Author

sgb004 commented Apr 1, 2022

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

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:

  • moving the script to the head: i know you noted this, but the script shoudl be placed at the end of the body, that shouldn't change and seeing the output change means something's not working as intended in the plugin
  • javascript in output: the console.log was originally wrapped in additional code, maybe that was a nice optimization? but without knowing that is a change in a newer version of webpack and HTML Plugin, all i see is that it's not properly compiling, and that something's not working

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

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

2 participants