Support webpack@5

[Austaras]

fixes #50

current status:

• es6-dynamic-import
• es6-externals
• es6-jquery
• es6-plain
• react
• typescript-lodash
• typescript-moment
• typescript-plain (at least it doesn't crash now)
• vue-cssextract
• vue-dynamic-import

[DanielSchaffer]

@Austaras when you've got a moment - can you rebase / re-commit / etc so that all the offline mirror changes are on their own in the first commit? Makes it easier to look at the actual code changes from GH.

If you haven't done this before, you can do a git reset 9def432 (9def432 is what develop is at now) on your local branch, which will unstage all of your committed branch changes. Then, reconstruct the commits and force push.

[Austaras]

Sure, but I will be pretty occupied for a week or two.

[DanielSchaffer]

@Austaras thanks, no rush!

[Austaras]

I've reorganized commits, there're still multiple places where I have absolute no idea how to deal with though

[Austaras]

Well, I tried to finish this before 2021 begins, but it seems to be unlikely to happen. Also due to sheer amount of code involved, I don't believe it's possible to support both webpack 4 & 5, so maybe this pr should be release with a new major version?

[Austaras]

@DanielSchaffer what does this this actually do? It doesn't seem to have any effect in every example

[DanielSchaffer]

@Austaras Unfortunately, I don't 100% recall why I did that. It may have something to do with the way the relative URL paths need to be set up with the dev server for each of the examples, particularly making sure they work the same way when there is only one example being run vs multiple. It's also entirely possible I was using the spaghetti + wall approach trying to debug something and forgot to remove it. That config is just for running the examples though, not for any real world build or reference.

[Austaras]

Aplogize in advance if you're refering to the deleted comment about publicPath, I figure it out myself(it's needed for handing publicPath, just need to update to adapt new Webpack options) and I deleted it thinking doing it should remove the notification also.

However I do want to know what extractCssChunks does

[DanielSchaffer]

No worries!

RE supporting both versions: I'm not opposed to setting up a maintenance branch for 4.x, but I'm a little wary of the administrative overhead for managing that. Are you aware of any stats regarding the adoption rate of v5?

Would it be easier to manage side-by-side support with abstractions rather than inline conditionals? For example:

public doTheThing() {
  if (IS_V5) {
    this.doItTheNewWay()
  } else {
    this.doItTheOldWay()
} 

vs

interface ThingDoer {
  doTheThing()
}

class V5ThingDoer implements ThingDoer {
  public doTheThing() {
    // doItTheNewWay()
  }
}

class V4ThingDoer implements ThingDoer {
  public doTheThing() {
    // doItTheOldWay()
  }
}

// elsewhere ...
let thingDoer: ThingDoer
... other version-specific services
if (IS_V5) {
  thingDoer = new V5ThingDoer()
} else {
  thingDoer = new V4ThingDoer()
}

...

thingDoer.doTheThing()

The abstraction would definitely help keep the code paths clearer, and would also make it easier to (at some point in the probably very distant future) drop support for v4 completely.

RE extractCssChunks - it's essentially to avoid ending up with duplicate CSS files - JS gets transpiled into different code, but the CSS that can be referenced from the source does not. See https://github.com/DanielSchaffer/webpack-babel-multi-target-plugin/blob/develop/src/normalize.css.chunks.plugin.ts#L10:L13 for a little more explanation

[Austaras]

For two branch: inline conditional is impossible because there will be likely two dozens of places needed to be patched, and although side-by-side support is possible, making TypeScript satisfied right is hard. Providing two version would be the most easy solution, but a putting legacy code into its own folder like html-webpack-plugin may also be possible

For extractCssChunks: since there will always be a legacy entry which has tagAssetsWithKey === false, L55 will always be run, so hasUntaggedTarget will always be true, thus under L89 and cleanCssChunks will never be run. What's the original purpose of this piece of code?

[DanielSchaffer]

making TypeScript satisfied right is hard

Can you elaborate on this? Using an abstraction like I described above would ideally remove the need for complicated union types (e.g. doTheThing(arg: V4Type | V5Type)), and allow the version-specific implementations to use only the types for their respective versions.

there will always be a legacy entry which has tagAssetsWithKey === false

This is inaccurate - while it is true of the default settings, it is possible to configure the plugin options so that both targets tag their assets (e.g. main.legacy.js, main.modern.js), which would otherwise result in separate CSS files (main.legacy.css, main.modern.css) that have identical content.

[Austaras]

The bigger problem is typing of webpack. Webpack 5 has its offical typing and has changed radically from webpack 4, so it's a lot of work to update the old webpack type definition. I don't know if it's possible for serperate files to refer to different webpack typing

[Austaras]

@DanielSchaffer what the purpose of this? I cannot figure out what it does and it really hard to find an alternative since mainTemplate.beforeStartup has been deprecated

[DanielSchaffer]

@Austaras had to look that one up - that all has to do with working around an issue with Safari: f8b1a40#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R101

[Austaras]

Oh I know, you're hooking into webpack's runtime to prevent multpile code being all excuted. However this is questionable because runtime: single will only generate one runtime for all the chunks.

Maybe we could ignore it for now and fix it later?

[DanielSchaffer]

Yeah, this feels like something that would probably need to be re-solved separately. IIRC it's also fixed in later versions of Safari, and 10.1 is coming up on 4 years and I can't find any global/national browser usage statistics that even list it, let alone showing any significant usage.

[Austaras]

Anyway, this is done

[Austaras]

@DanielSchaffer could you take some time to review this? It's a huge patch, but changes are mainly in targeting.plugin.ts

[cruzdanilo]

can i help with something here?

[florinvasilevilsan]

When will this be merged?