Output a UMD library instead of the default var library

[agjohnson]

This is another change that will require some testing, but I'm mostly
certain it does change anything about the current usage.

The default library target for Webpack is "var", which defines a global
variable, named window.ethicalads in this case. It, however, does not
export anything as a normal Node library would, so is not usable from
NPM directly.

This changes the library type to UMD, which in the Webpack config:

"umd" ... exposes your library under all the module definitions, allowing it to work with CommonJS, AMD and as global variable

So, this seems to me to be only additive to the current configuration (global
variable). It's very worth some testing though.

This would also make it possible to package on NPM and install as a
library, though we'll need to tune up the automatic execution code as we
found this week on the RTD JS library work.

Requires #154

[agjohnson]

Actually, because this library functions similar, I found a extension to this pattern here in readthedocs/doc-diff#9

That PR creates two libraries for output, one that automatically executes (for browser usage) and one that doesn't (for usage as a Node library).

I found that the logic here and in doc-diff to detect this doesn't actually work for Webpack and especially ES modules. The logic in both is if (require.main === module), but the use cases for this are pretty limited.

This PR can be a standalone change, but there are more changes to make a library that doesn't automatically execute.

[davidfischer]

I don't fully understand this change, but I did test this manually in a browser context and there was no issues with it. There's no significant change in filesize either.

[agjohnson]

For a little more context here, the default for Webpack is libraryTarget: "var", which only sets a global/window variable for everything normally exported from the library. This is why the library currently can't be used as a library installed from NPM, because var target libraries don't actually export anything.

It seems umd library type basically supports all of the patterns at once -- it sets a window/global variable but also exports all exported functions/classes/etc. This is what is required to do a import * as ethicalads from "ethical-ad-client";

[humitos]

Is there anything blocking merging this PR here?

[agjohnson]

There shouldn't be, other than a rebase. I wasn't sure exactly how to verify this doesn't break anything, so would probably want to leave merge and release up to @davidfischer.

I'll rebase this at least for now.

[davidfischer]

I reviewed it and it is good to merge. I'll aim to merge and release it this week.

[agjohnson]

I'll be away next week, in case this leaks into next week, but users should be able to use the library through a window global still, and should also be able to use the library through and npm install as well (which should be new). So, I wouldn't expect any difference yet. Any users installing the package from the repo URL and doing something with the library directly should have an easier time doing so, but it would be worth verifying this.

[davidfischer]

Any users installing the package from the repo URL and doing something with the library directly should have an easier time doing so, but it would be worth verifying this.

I'll run a quick check but I want to limit the different ways we need to support things as much as possible so not sure we'll advertise this.

[humitos]

Besides this change, in my PR I had to remove this chunk of code because it seems the auto-detection of being imported as a module does not work fine: https://github.com/readthedocs/ethical-ad-client/pull/148/files#diff-e727e4bdf3657fd1d798edcd6b099d6e092f8573cba266154583a746bba0f346L742

What's the best way to test/QA that code to find out how to make it work in a reliable way?

[agjohnson]

I forget where I noted it, but that code will never work in a reliable way as there isn't really a good pattern anymore for detecting when a library is being imported vs requested via a <script>. There are some changes to Node/ECMA that would help, but they were farther out.

If we want a library to both automatically initialize and never automatically initialize, depending on how it is used, I've been outputting two libraries.

[humitos]

I forget where I noted it, but that code will never work in a reliable way as there isn't really a good pattern anymore for detecting when a library is being imported vs requested via a <script>. There are some changes to Node/ECMA that would help, but they were farther out.

I found these two SO questions and I understand, as you said, it's not possible:

• https://stackoverflow.com/questions/61797713/how-to-programmatically-determine-within-the-module-if-javascript-module-was-l
• https://stackoverflow.com/questions/61790963/how-to-determine-if-javascript-module-was-imported-or-loaded-via-script-src

If we want a library to both automatically initialize and never automatically initialize, depending on how it is used, I've been outputting two libraries.

Can you expand a little more on this? What are the required changes? How the addons should import this module and how publishers should import this module (note that we cannot change anything on their side, I guess)?

[humitos]

from our Slack conversation,

I'd avoid having two different files if that's possible. So, I'd suggest something like using a variable global.AUTOLOAD_PLACEMENT that defaults to true (so nothing is changed in your side) and we can define it as false from ours... but I don't know if that's possible

@agjohnson would that be possible?

[humitos]

I was able to use this PR as-is. So, I think we can merge it and it will work as we expect.

There are some notes that I want to mention here, tho:

• we require to install sass, sass-loader and style-loader in the addons
• we need to mark the div we automatically create with data-ea-manual: true
• users integrating with us will need to mark data-ea-manual: true --which I think is the way it's documented, so 👍🏼
• I created a new Webpack rule to be able to "compile" .scss files

I made this changes in readthedocs/addons#81

That said, I think we are 💯 to move forward and merge this PR.

[agjohnson]

Can you expand a little more on this? What are the required changes?

Webpack would need configured to output two bundles. One bundle has the code for automatic loading of placements, and is the bundle that EA would serve for pageviews outside Read the Docs. The other bundle looks similar, but does not make the same autoload method/function call on load.

The reason for outputting two bundles is so that the EA client can perform all of the bundling operations, like compiling its SASS sources, and the addons client doesn't need to replicate this (or anything else the EA client webpack configuration is doing).

@agjohnson would that be possible?

It would take some Webpack hacks on the addons library bundles, but a global might be possible in combination with Webpack ProvidesPlugin. Assuming we don't set a default in the EA client library, I think this might work okay, but would be worth a test especially for reusing the EA client built bundles.

---

We can still proceed with this PR though, all of these decisions still require the change in this PR either way.

[agjohnson]

I feel like this should probably still move forward, it's a small change.

Ultimately, we should be installing addons dependencies through packaging, to ensure backwards compatibility between addons and the EA client. This change is required for that, as is a change to avoid auto injection of placements.