brunocodutra / webapp-webpack-plugin

[DEPRECATED] use favicons-webpack-plugin instead
https://www.npmjs.com/package/webapp-webpack-plugin
MIT License
125 stars 17 forks source link

Convenient way to get generated html tags from js #156

Closed vonagam closed 5 years ago

vonagam commented 5 years ago

So, this plugin works well with html-webpack-plugin, but for server side rendering with react i needed to get generated markup directly. Something like this:

import favicons from './path/to/favicon';

It is manageable to create module rule for handling this using available hook and custom loader. But it doesn't feel convenient enough.

I propose addition to the plugin: an instance method rule which generates relevant module rule. So that in a config you can write this to get access to tags by requiring favicon source:

let plugin = new WebappWebpackPlugin(...);
config.plugins.push( plugin );
config.module.rules.push( plugin.rule() );

Here is my working implementation (used with webpack 4, should work with 3 too).

There two issues:

1) To get absolute path of a favicon source for rule.include from options.logo i used path.resolve, which does not really match webpack's path resolution mentioned in readme.

2) I have not added tests.

What do you think?

brunocodutra commented 5 years ago

This is definitely an interesting idea!

I had a look at your implementation and I noticed that:

  1. it uses global state as a means to share data;
  2. It implicitly relies on the fact the plugin completes before the loader is executed;

This makes it very brittle and hard to maintain.

We can actually do much better, because WebappWebpackPlugin already relies on a loader internally! All we need to do is to write a thin layer on top of it that forwards the arguments it needs and decodes its output to extract the HTML.

vonagam commented 5 years ago

1) It It is more like class or singleton state than global. If webpack allowed loaders to be a function in a config and not a string, then i would have used this option. But this is best that i've come up with so far.

2) It relies on that fact. But maybe not so implicitly, because loader data gets populated in tapping to compiler.make hook. Because it worked for me i assume that this hook is executed and waited for before main loaders get their turn (i may be wrong though, docs do not help, here this hook's place in webpack's source code).

3) I saw that loader before. But it does compilation done and currently there is no caching inside this loader itself, so i didn't want to execute favicons twice. Plus this loader itself does not take into account results of possible modifications in webappWebpackPluginBeforeEmit hook.

vonagam commented 5 years ago

Updated implementation. No global/class/singleton state now.

brunocodutra commented 5 years ago

It It is more like class or singleton state than global.

Which is just a fancier term for global state :)

Because it worked for me i assume that this hook is executed and waited for before main loaders get their turn

That's what make me uncomfortable with this approach - I'm not convinced that compiler.make is guaranteed to complete before the loaders are executed, but that should be easy to check empirically: could you try commenting out return callback(); and replacing this.ruleTags = tags; by

setTimeout(() => {
  this.ruleTags = tags;
  return callback();
}, 10000);

and check whether it still works?

I saw that loader before. But it does compilation done and currently there is no caching inside this loader itself, so i didn't want to execute favicons twice.

The loader itself doesn't need to implement caching, since we use cache-loader - my suggestion was to do the same thing as part of the rule and use cache-loader in conjunction with our internal loader, which would hopefully avoid generating favicons twice.

Plus this loader itself does not take into account results of possible modifications in webappWebpackPluginBeforeEmit hook.

That's a good point though.

Updated implementation. No global/class/singleton state now.

Well done, looks much better now. Though I believe you might have missed the fact that ruleIdent is not doing anything and may be completely removed.

To summarize I'm not opposed to implement it your way, provided we can demonstrate that the plugin is guaranteed to complete before the loader runs and we also need to verify this for Webpack 2 and 3 which are still supported, otherwise we need a new major version to release this feature.

There is one open question though: how do we handle the fact that we cannot resolve the absolute path to the logo the same way the plugin does? The only thing I can think of is to require the path provided by the user to be absolute if rule() is used and in this case we need to assert that it is indeed absolute and throw a nice error message explaining this requirement to the user.

vonagam commented 5 years ago

Updated implementation.

setTimeout test showed that compiler.make timing is not a guarantee, so i made ruleLoader async and added tags promise property to a plugin instance. Now relying is explicit.

ident was for helping webpack (in webpack's source, clarified in docs), but in this case it is optional as JSON.stringify will handle plugin ok. Removed.

Webpack 2 and 3

It is 3 and 4. I tested on 4 (building and dev serving).

Yeah, i think there is no other way except throwing an error here. Added the assertion.

brunocodutra commented 5 years ago

Updated implementation.

Looks great, would you be interested in creating a PR to help us discuss the last details? Also, it would be great if you could add a unit test to cover this feature so we can merge it.

It is 3 and 4.

Geez you're right, I changed it only 7 days ago and already forgot I did it.

brunocodutra commented 5 years ago

Closed by #158