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

Bundles are created in isolation (not suitable for webcomponents) #379

Closed
daKmoR opened this issue Dec 3, 2018 · 29 comments
Closed

Bundles are created in isolation (not suitable for webcomponents) #379

daKmoR opened this issue Dec 3, 2018 · 29 comments
Milestone

Comments

@daKmoR
Copy link
Collaborator

daKmoR commented Dec 3, 2018

Each test file get's bundled individually but placed in a single browser window.
This leads to errors when using a global registry or global variables/systems that need to be unique.

actual behavior

Error in test Failed to execute 'define' on 'CustomElementRegistry': this name has already been used with this registry

expected behavior

Test run successfully even if some global variable is used

What happens

  1. foo-one has a test file
  2. load bundle one
  3. foo-one gets registered
  4. all good
  5. foo-two has a test file
  6. load bundle two
  7. foo-two has a dependency on foo-one
  8. foo-one is included (again) in bundle (as bundles are created in isolation)
  9. => ERROR Failed to execute 'define' on 'CustomElementRegistry': this name has already been used with this registry

Steps to Reproduce

I made a minimum failing test:
https://github.com/daKmoR/karma-webpack-wc-bug

git clone https://github.com/daKmoR/karma-webpack-wc-bug.git
cd karma-webpack-wc-bug
npm install
npm run test

possible Solutions

These are mere ideas to spark a conversation:

1) manually create a single entry point

Besides the *.test.js files have also an index.js

Good

  • easily doable
  • no change in karma-webpack needed

Bad

  • can't load individual test files
  • cumbersome => need to add file + add to index.js
  • not really sadisfying Developer Experience

2) treat it as "Multi Page Application"

Use https://webpack.js.org/concepts/entry-points/#multi-page-application to create bundles that know of each other.
E.g. do not create bundles in isolation.

Good

  • easy to use
  • can load individual test files
  • just "works" and therfore sadisfying Developer Experience
  • smaller test bundles

Bad

  • change in karma-webpack needed
  • unsure how complex the change will be

System Info

$ npx https://gist.github.com/daKmoR/b509ed34576a488ea79d3ac66ccab74e
npx: installed 1 in 5.603s
$ node --version
v10.9.0

$ npm --version
6.4.1

$ cat /proc/version:
It is a WSL (Windows Subystem Linux)
Linux version 4.4.0-17134-Microsoft (Microsoft@Microsoft.com) (gcc version 5.4.0 (GCC) ) #345-Microsoft Wed Sep 19 17:47:00 PST 2018

$ lsb_release -a:
Distributor ID: Ubuntu
Description:    Ubuntu 16.04.3 LTS
Release:        16.04
Codename:       xenial
@dogoku
Copy link

dogoku commented Dec 3, 2018

Just adding my 2 cents.

We were also facing the same issue when switching from browserify to webpack and we were surprised when we noticed this behaviour.

In the end we opted for using karma-iframes, which runs each bundle in an isolated iframe. It works as expected and it's actually a clever solution to this and other problems, but it's not "ideal" for a few reasons:

  • Additional dependency (even though maintainer has been quite responsive) and configuration
  • karma-webpack still does the extra work of bundling everything for each test file

While I understand it's not exactly apples-to-apples, karma-browserify solved the same problem for browserify, by creating a single bundle and then importing all the matched test files via stub layer, ensuring that everything is called once. See https://github.com/nikku/karma-browserify#how-it-works

@daKmoR
Copy link
Collaborator Author

daKmoR commented Dec 3, 2018

@dogoku thxxxx :) that sounds like a good workaround :)

If we can't agree on a solution via karma-webpack that's probably the way we are going to do too. However, I still hope we can make a proper solution within the plugin itself (because of the reasons you mentioned).

So the question is could this "Multi Page Application Solution" work?
Me and @LarsDenBakker are not afraid to do some work on karma-webpack itself if we agree on a path forward. Of course some hints would be nice :)

@matthieu-foucault
Copy link
Collaborator

Me and @LarsDenBakker are not afraid to do some work on karma-webpack itself

😍

This project badly needs maintainers, so that's great if you can help!

I'd say option 2 is the way to go, as it mirrors Karma's behavior. In terms of hints... Well I don't have any, I've mainly been putting out fires here, and haven't had time to take an in-depth look into the project 😅

Maybe @rynclark , @michael-ciniawsky or @d3viant0ne could quickly chime in 👋

@daKmoR
Copy link
Collaborator Author

daKmoR commented Dec 4, 2018

I will post my progress here so if I am straying off just let me know.

First I wanted to verify what an actual working webpack config should look.

So that should be fine

  entry: {
    one: './foo-one.js',
    two: './foo-two.js',
  },
  output: {
    filename: '[name].bundle.js',
  },
  optimization: {
    splitChunks: {
      chunks: 'all',
      minSize: 0,
      cacheGroups: {
        commons: {
          name: 'commons',
          chunks: 'initial',
          minChunks: 2,
        },
      },
    },
  },

Exact config used: https://github.com/daKmoR/karma-webpack-wc-bug/blob/webpackconfig/webpack.config.js

Output will be 3 files

  • commons.bundle.js (shared stuff e.g. foo-one)
  • one.bundle.js (load commons)
  • two.bundle.js (load commons + foo-two

So that is exactly what we need - everything that is used more then once get's pushed to a shared commons.bundle.js. e.g. nothing will be declared double as it will only be "required"

See here the actual files: https://github.com/daKmoR/karma-webpack-wc-bug/tree/webpackconfig/dist

@matthieu-foucault
Copy link
Collaborator

Take a look at this comment in #22
Is this still the case with the current Karma version? Can you have two input files and three output files?

@daKmoR
Copy link
Collaborator Author

daKmoR commented Dec 5, 2018

yes that seems like it... 😒
so I guess the only way to solve this will be to "refactor" to a become a karma framework... doing so, however, requires probably a completely different workflow.

I have been playing around a little and it seems we could just read the karma files and totally replace them with whatever webpack creates. Doing so should allow for a much easier interaction as we could provide the entry points simply via the webpack option instead of manually adding the entry and generating our own SingleEntryDependency.

However, this strays pretty far from the original plugin now 🙈 So I am afraid if I continue that it's more like a rewrite then a bugfix - would you be at all will to accept such a huge change?
I would of course explain each taken decission why and how... non the less it would probably be a huge breaking change... e.g. requires action from every user to add it as framework - so maybe 5.0.0?

Nonetheless here is the progress for today:

  • node script
  • configures webpack only via options
  • adds webpack plugin to have a possibility to interact

intersting parts

// webpack plugin
class KarmaSyncPlugin {
  apply(compiler) {
    compiler.hooks.done.tap('KarmaSyncPlugin', stats => {
      this.notifyKarmaAboutChanges();
    });
  }
  // ...
}

const webpackOptions = {
  mode: 'development',
  plugins: [
    new KarmaSyncPlugin({ karmaEmitter: /* here karma will pass it's emitter */ })
  ],
  // ...
};

class KarmaWebpack {
  constructor() {
    const compiler = webpack(webpackOptions);
  // ...

==>> Full file <<==

ToDos:

  • have working webpack config
  • create a node script for webpack with needed hooks
  • karma framework plugin
  • "fake" karma preproccesor (e.g. only to mark files to be added to webpack in the framework step)
  • webpackMiddleware interaction (webpack is more in control - will the WebpackBlocker still be needed? )

@dogoku
Copy link

dogoku commented Dec 5, 2018

I guess this is a question for the maintainer(s), really.

As a user, I believe the benefits (speed, isolation) far outweigh the inconvenience of a breaking change, assuming there won't be unnecessary additional setup (e.g requiring the user to make fake entry files, when it should be done by Karma).

In any case, @daKmoR i am more than happy to help test it out on our projects, which have vanilla web components and 1000s of tests between them.

@ryanclark
Copy link
Collaborator

ryanclark commented Dec 6, 2018 via email

@ryanclark
Copy link
Collaborator

Sorry I wrote that previous comment via email and it doesn’t seem to have applied any formatting...

@daKmoR
Copy link
Collaborator Author

daKmoR commented Dec 6, 2018

@dogoku yes I think so too :) that's why I'm investing my evenings here :p (of course also to learn) and thanks for the encouraging words :) that motivates me even more :) so once something is ready I will definitely let u know.
[start shameless promotion]
if you do work with webcomponents this might also be intersting for you https://open-wc.org/
[end shameless promotion]

@rynclark I am still playing around with some hard coded files and sort of hacking it in the "node_modules" folder and so far it seem pretty doable (at least I am still optimistic) Once I have something to really review I will definitely let you know.

You can probably follow me along while I am working on this pull request: https://github.com/daKmoR/karma-webpack-wc-bug/pull/1/files
This outlines the idea so you can take a look for it... but like I said by far nothing to really consider yet.

@ryanclark
Copy link
Collaborator

@daKmoR thanks for your work on this - if you need me at all DM me on twitter or something, not sure what everyone uses these days (apart from gitter please ha)

@dogoku
Copy link

dogoku commented Dec 6, 2018

if you do work with webcomponents this might also be intersting for you https://open-wc.org/

@daKmoR I have already seen open-wc yesterday as I was checking your contribs (feels like a facebook stalker) and it's really impressive the amount of knowledge you have managed to collect! I actually wanted to ask about a couple of things (specifically around storybook integration) but I figured this isn't the thread to do so.

@daKmoR
Copy link
Collaborator Author

daKmoR commented Dec 6, 2018

@rynclark thx will let you know :)

@dogoku thx :) we have an #open-wc channel in polymer.slack.com or I have twitter but oh boy I haven't posted anything in forever - still works for private messages 😁

@daKmoR
Copy link
Collaborator Author

daKmoR commented Dec 8, 2018

quick update
the hardcoded test files are now running and correctly report the test results.

I had to add to webpack

  optimization: {
    runtimeChunk: 'single',

and load the file runtime.bundle.js to make sure that everything is imported only once.

So getting it now to work for a "normal" karma config should be doable... I'm only a little worried for the interaction with other plugins especially for code coverage... as the code is now split into multiple files and I am not sure to how to make these extra files nicely available to everywhere 🙈

but let's see - I will probably try out the current existing workaround in karma-webpack to read the webpack generated file in a karma preprocessor... combined with the inline source maps that webpack generates it could work... let's see.

@daKmoR
Copy link
Collaborator Author

daKmoR commented Dec 8, 2018

Update of today:

  • So I have a very rough prototype working here
  • It uses a similar "concept" as karma-browserify by using the first preprocessor to start the bundling process
  • Different to karma-browserify it still uses multiple files / entrypoints (which should make rebundling really fast)
  • It uses a framework plugin to setup the shared files

Still missing:

  • bundling needs to be debounced (as karma file refresh currently retriggers bundling)
  • even in watch mode a file change currently does not trigger an update
  • bundle files are created within a local folder .karma-cache => will either need to manage (e.g. reset/cleanup) the folder or move to an os temp folder

@daKmoR
Copy link
Collaborator Author

daKmoR commented Dec 9, 2018

Added features:

  • debounce bundling (only one compile run now 🎉 )
  • support for karma watch mode was actually already working (while switching to prepocessors)
  • support for webpack watch mode (using webpacks own watch mode - e.g. no need of a custom middleware 💪 )
  • bundle files are created in the os temp folder (using a virtual filesystem was not worth the effort as we at least the shared files on the filesystem for karma anyways - could be revisited if improvements are needed)

Overall it now fully supports all core features I would expect 🎉

  • test files are usually super small
  • most code is within the shared bundle (like chai, testhelpers, shared dependencies, ...)
  • watch mode is fast and works without any extra middleware
  • debugging works nicely in the cases I tested
  • way simpler code imho

@rynclark can you pls take a look at the 2 files KarmaWebpack Class and karma-webpack plugin

If the solutions seems good I can prepeare a Pull Request to karma-webpack itself.

PS: maybe we could then also get rid of the build step in karma-webpack? I think only supporting somewhat up to date node versions should be good enough. However then again that is a different topic.

@dogoku
Copy link

dogoku commented Dec 9, 2018

Wow, man you blew me away. This looks amazing work for such a short time. I'll test it out tomorrow and give you feedback.

I'll try to run it on both mac & windows, and run both single-run and debug modes.

@daKmoR if you need me to test anything in particular, let me know.

@daKmoR
Copy link
Collaborator Author

daKmoR commented Dec 9, 2018

@dogoku thank you for your kind words ❤️ this is exactly the reason why I love open source :) thank you :)

I will prepare a Pull Request so it's also easier to test :)

Nothing particular to tests... just be careful with custom configs... also you will need to make sure webpack is added as a framework and preprocessor.

Still mind you I would still consider this as a POC as it's missing tests, linting, ... but it's a good starting point as we now know it's possible :)

@daKmoR daKmoR mentioned this issue Dec 9, 2018
3 tasks
@matthieu-foucault
Copy link
Collaborator

matthieu-foucault commented Dec 9, 2018

I would still consider this as a POC as it's missing tests

So does the master branch 😅

I created a 5.0.0 branch, so you can make your PR against that one

I'll try to fix the remaining bugs in 4.0.0 soon so that we can be done with that and focus on your implementation.

@matthieu-foucault
Copy link
Collaborator

matthieu-foucault commented Dec 9, 2018

I think only supporting somewhat up to date node versions should be good enough

We have to match webpack here. So currently >= 6.11.5

@daKmoR
Copy link
Collaborator Author

daKmoR commented Dec 9, 2018

I created a 5.0.0 branch, so you can make your PR against that one

changed

I'll try to fix the remaining bugs in 4.0.0 soon so that we can be done with that and focus on your implementation.

could we maybe release a 5.0.0-rc1 in a somewhat timely manner e.g. within a week or so? I would like to test it on a broader scale and a released version makes it way easier :)

We have to match webpack here. So currently >= 6.11.5

I don't think there is anything used that needs a more up to date version?
=> I looked at karma-browserify and they currently also do not use a build step
=> test should show... so probably add them sooner then later (I started with one already)
=> In the long run we could readd babel and write everything in "pure" es modules so then transpilation would make sense again

@matthieu-foucault
Copy link
Collaborator

  • We can have a 5.0.0-rc1 by the end of next weekend
  • I would rather convert the source to Typescript than re-add babel (see Add type checking to source files #378 ), that will prevent many future issues.

@ryanclark
Copy link
Collaborator

Does this need to be moved to 5.0.0? As we haven't released 4.0.0 yet, we're not technically making a breaking change from latest.

@ryanclark
Copy link
Collaborator

(p.s. awesome work)

@matthieu-foucault
Copy link
Collaborator

Well some people started using the release candidates of 4.0.0 already, I don't think such a change should happen after a RC.
We can release 4.0.0 this week, and work on relasing v5 asap

@daKmoR
Copy link
Collaborator Author

daKmoR commented Dec 9, 2018

I would rather convert the source to Typescript than re-add babel (see #378 ), that will prevent many future issues.

also a possibility... so let's go without babel for now and think about that one afterwards

(p.s. awesome work)

THXXX 💪

@ryanclark
Copy link
Collaborator

True @matthieu-foucault. What's left for 4.0.0? Then let's get onto 5.0.0. Nit pick - can you change the 5.0.0 branch name to be next please? Just to follow convention. Versioned branches are for older versions.

@ryanclark
Copy link
Collaborator

I know that implies that master is currently latest on npm, but we might as well release (so that bit is true) and get next as v5

@matthieu-foucault
Copy link
Collaborator

matthieu-foucault commented Dec 9, 2018

@matthieu-foucault matthieu-foucault added this to the 5.0.0 milestone Dec 9, 2018
matthieu-foucault pushed a commit that referenced this issue Dec 13, 2018
…380)



Closes #379 
BREAKING CHANGE: webpack needs to be added to frameworks

```
// old:
frameworks: ['mocha'],

// new:
frameworks: ['mocha', 'webpack'],
```

BREAKING CHANGE: old alternative usage is no longer recommended
BREAKING CHANGE: webpack-dev-middleware removed
BREAKING CHANGE: default webpack configuration changed drastically
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants