vue-concurrency icon indicating copy to clipboard operation
vue-concurrency copied to clipboard

Webpack 5 fails due to the way CAF is imported

Open lllopo opened this issue 3 years ago • 45 comments

There is a conflict in the way how CAF is imported on upgrade from Webpack 4 -> 5.

In TaskInstance.ts the current import CAF from "caf"; works with Webpack 4, but crashes on Webpack 5. For Webpack 5 the code that works is import { CAF } from "caf";.

Edit : I actually can't check if the stricter import that works in Webpack 5 is working on v4 too or not. If it is - then it is an easy fix.

lllopo avatar Jan 08 '22 13:01 lllopo

And also - is the project still alive ?

lllopo avatar Jan 14 '22 14:01 lllopo

Hey @lllopo a minimal reproduction repo would be very useful here 🙏 I'll try to see if import { CAF } from "caf"; works elsewhere than Webpack 5 but otherwise all we can do is to propagate the issue either to webpack or caf

MartinMalinda avatar Jan 14 '22 16:01 MartinMalinda

I'm trying import { CAF } locally and tests are not passing.

Looking at

https://github.com/getify/CAF/blob/master/src/caf.js#L13

It looks like a webpack issue to me. It should clearly be a default export so I'm not sure what's going sideways on webpack side.

MartinMalinda avatar Jan 14 '22 16:01 MartinMalinda

Unfortunately I don't have a repo. I was digging around the error I'm getting, though, and found this : https://issueexplorer.com/issue/getify/CAF/25. I immediately thought this is my case. So maybe you can get some idea what is going on the issue above. I also found a few explanations and ideas here : https://stackoverflow.com/questions/50435253/webpack-imported-module-is-not-a-constructor ... so it looks more like a Webpack 5 "feature" than an "issue" :-). As a proof I just edited the TaskInstance.ts in node_modules (stupid idea, I know), made my code import from /src instead of /dist and configured Webapck to build vue-concurency from the TS sources ... and it definitely works. Biggest question is how to make it work in both Webpack 4 and 5. Someone suggested using "require" instead of "import" in the stackoverflow issue. Maybe this would help.

lllopo avatar Jan 14 '22 21:01 lllopo

@MartinMalinda Checked the CAF sources. They are using module.exports, which means no default export (or not exactly). This means import CAF from "caf" will never work in Webpack 5 and rightly so. Very unfortunate, but what I did is to try the "required" approach. This :

const CAF = require("caf/caf");

works in my setup with Webpack 5. So if it works on yours too (supposedly it should), I'd rather say go for it.

lllopo avatar Jan 15 '22 12:01 lllopo

Thanks @lllopo . I'll check ifrequire() does not break vite (it could because that doesn't look like it's too browser friendly).

MartinMalinda avatar Jan 19 '22 15:01 MartinMalinda

It looks like it builds 🎉 @lllopo do you use Vue 2 or Vue 3? With latest Vue 3 version I know have some problems with TS on master, which is un unralted issue. I can publish a Vue 2 release for you though.

MartinMalinda avatar Jan 19 '22 15:01 MartinMalinda

@MartinMalinda I'm using latest Vue 3.

lllopo avatar Jan 19 '22 16:01 lllopo

@lllopo I couldn't fix this in v3 which uses EffectScope from Vue 3 for some extra benefits. Unfortunately there's some microbundle + TS that shows up with Vue 3.2 onwards.

But I released 2.3.0

Once I can figure out the build issue or maybe once I migrate from microbundle to vite or other tool I'll fix this in v3 as well.

MartinMalinda avatar Jan 19 '22 16:01 MartinMalinda

Ok, I'll be looking forward to it. Just include it with the next successful build, whenever it goes out, please. As of now I'll be running my tweaked version, but it will be overwritten as soon as you release a new one, so it would be great if I don't have to apply the fix manually again.

lllopo avatar Jan 19 '22 16:01 lllopo

Maybe I didn't explain it too clearly.

I released 2.3.0 which at the moment is latest stable version of vue-concurrency and it supports Vue 3.

I have some 3.X.X pre-releases which support newest features from Vue 3.2+ but because of those build issues it didn't move from the pre release stage yet.

Chances are you're running 2.X.X version of vue-concurency currently.

But you can of course wait till this is fixed and released in stable 3.X version.

MartinMalinda avatar Jan 19 '22 17:01 MartinMalinda

No, no ... I think I got you right. I just re-checked my package.json. I'm actually even using 4.0.0-1 right now. No worries. I'm also on very-very early stages of the project where I'm using it ... I can even call it experimental stage :-). Fingers crossed you will solve the issues you have. It transpiles fine at my place, but it is probably because I'm using it on different bundler (or maybe different tsconfig).

lllopo avatar Jan 19 '22 17:01 lllopo

Uncaught ReferenceError: require is not defined unfortunately I have to revert this :(

MartinMalinda avatar Jan 20 '22 17:01 MartinMalinda

https://github.com/getify/CAF/issues/25#issuecomment-1017802252

MartinMalinda avatar Jan 20 '22 18:01 MartinMalinda

Uncaught ReferenceError: require is not defined unfortunately I have to revert this :(

Ahhh ... bad. Maybe some kind of polyfill could solve this ?

lllopo avatar Jan 20 '22 18:01 lllopo

Here's what the ESM distribution of CAF looks like:

index.mjs:

export {default as CAF} from "./caf.mjs";
export {default as CAG} from "./cag.mjs";
export {default as CAFShared} from "./shared.mjs";

caf.mjs:

export default Object.assign(CAF,{
   cancelToken:cancelToken,
   delay:delay,
   timeout:timeout,
   signalRace:signalRace,
   signalAll:signalAll,
   tokenCycle:tokenCycle
});

As far as I'm aware, you can thus, as spelled out here, validly import the CAF function/object in one of two ways:

import { CAF } from "caf";

// or

import CAF from "caf/caf";

I think this is perfectly valid ESM, and I'm not aware of anyone else having issues using CAF in ESM projects.


Is WebPack (WP5 or WP6?) trying to convert CAF to UMD, or is it trying to leave it as ESM but simply bundle it together?

If it's trying to convert it, it should just instead be pointed at the UMD distribution of CAF, included in the dist/caf directory.

If it's trying to keep CAF as ESM, but bundle it into a single ESM file, it seems like it would be better to reference it as import CAF from "caf/caf" rather than to use the index re-export approach with import { CAF } from "caf".

IOW, I'm not sure if WP5 is having trouble with the Object.assign(..) so much as it might be having trouble with the export { default as .. } from .. pattern?

Does that make sense?

getify avatar Jan 20 '22 21:01 getify

This :

import CAF from "./../node_modules/caf/dist/esm/caf.mjs";

is a crazy construct, but also builds fine with WP5. It is pretty much the equivalent of the 'require' that was not working, but without 'require'. Would you try it out ?

Edit : basically, the idea is to import from a module that exports only CAF (without the CAG and others) and then WP5 is happy with the import CAF from ... part.

Edit 2: Note that import CAF from "caf/dist/esm/caf.mjs"; which should be the same is not working for some reason.

lllopo avatar Jan 21 '22 08:01 lllopo

I'm curious why you have to use such an explicit file path in the from specifier? Is WP not able to understand the import-entry-points from the npm package, like "caf/caf" or "caf/cag"? Does WP require all your import statements to use explicit file paths, or does it just not like CAF's exports?

If at all possible, CAF should be imported as one of these two:

import { CAF } from "caf";

import CAF from "caf/caf";

I'd discourage (unless absolutely required -- but again, why!?) relying on paths, as that creates future potential breakage if I for example re-arrange the structure or names of files. The named import-entry-points are a beneficial abstraction to avoid such issues should something like that ever need to occur.

getify avatar Jan 21 '22 09:01 getify

Side note: I am considering changing that "caf/caf" import-entry-point to "caf/core" soon, to be less confusing. But I won't be removing the "caf/caf" entry-point probably ever, so it's pretty future proof.

getify avatar Jan 21 '22 09:01 getify

I'm curious why you have to use such an explicit file path in the from specifier? Is WP not able to understand the import-entry-points from the npm package, like "caf/caf" or "caf/cag"? Does WP require all your import statements to use explicit file paths, or does it just not like CAF's exports?

If at all possible, CAF should be imported as one of these two:

import { CAF } from "caf";

import CAF from "caf/caf";

I'd discourage (unless absolutely required -- but again, why!?) relying on paths, as that creates future potential breakage if I for example re-arrange the structure or names of files. The named import-entry-points are a beneficial abstraction to avoid such issues should something like that ever need to occur.

I'm not that good at it at all, so most of the things I mention are something of a guess. Still - as far as I understand it, module.exports = { A, B, C } makes the default export to be the object in question { A, B, C }. So in our case module.exports = {CAF, CAG, ... } means that import CAF from "caf" will result CAF to be equal to {CAF, CAG, ... } instead of CAF itself. So, WP5 properly imports the right thing which means it won't work since we actually need the CAF and not the object holding it all {CAF, CAG, ... }, imo. If so, import {CAF} from "caf" is the proper way to import CAF indeed. Still, if this is correct, why import CAF from "caf" is working fine in WP4 and others - I have no idea. If this is not correct on other hand, why WP5 is not working then - also no idea :-).

lllopo avatar Jan 21 '22 09:01 lllopo

As of why not import CAF from "caf/caf"; ? it works perfectly fine (and it should indeed) when I do it on my root project, but when I try it in the node_modules vue-concurency project it fails with "module not found" ... and that's why I came up with this crazy path thing. As I said - I'm not that good at it, so maybe It is just because I'm tweaking a "subproject". I guess @MartinMalinda can try it if it works fine at his side.

lllopo avatar Jan 21 '22 09:01 lllopo

Not working due to 'caf/caf' import can't you use just 'caf' from npm if works with simple import import Caf from 'caf';

vdvibhu20 avatar Jan 29 '22 07:01 vdvibhu20

@vdvibhu20

Not working due to 'caf/caf' import

What do you mean? This works perfectly fine as far as I can tell:

import CAF from "caf/caf"

if works with simple import

import Caf from 'caf';

No, that's not what you want. You should pick one of these two:

import CAF from "caf/caf"

// or:

import { CAF } from "caf"

import Caf from "caf" actually would "work" but in a surprising/weird way. You'd have a namespace called Caf that had properties on it named CAF and CAG... so then to use the library, you'd have to do in code x = Caf.CAF(..). I advise against that.

getify avatar Jan 29 '22 18:01 getify

@getify

I still don't completely understand how import { CAF } from "caf" works in combination with

export default Object.assign(CAF,{
   cancelToken:cancelToken,
   delay:delay,
   timeout:timeout,
   signalRace:signalRace,
   signalAll:signalAll,
   tokenCycle:tokenCycle
});

Here vue-c: imports it like this: https://github.com/MartinMalinda/vue-concurrency/blob/master/src/TaskInstance.ts#L1

And then uses like this: https://github.com/MartinMalinda/vue-concurrency/blob/master/src/TaskInstance.ts#L184

Therefore the default import is calleable and it works.

And it makes sense, no? CAF is a function: https://github.com/getify/CAF/blob/master/src/caf.js#L32

Object.assign assigns these to the function itself, eg CAF.cancelToken = cancelToken and so on. It does not lead to an object like { CAF, cancelToken, delay... }. So I can't extract CAF from CAF function itself.

Object.assign(CAF,{
   cancelToken:cancelToken,
   delay:delay,
   timeout:timeout,
   signalRace:signalRace,
   signalAll:signalAll,
   tokenCycle:tokenCycle
});

What's the reason for Object.assign ? Mutating a function like this just before exporting also seems a tiny bit strange. export default { CAF, cancelToken, delay, timeout, signalRace, signalAll, tokenCycle } would be more clear, no? Unless we really want these as properties on the function itself.

MartinMalinda avatar Jan 31 '22 15:01 MartinMalinda

@MartinMalinda I'll try to put my 5 cents on this again (feel free to correct me if I'm wrong and sorry in advance, if I'm talking nonsense). If you look at CAF's package.json, the default export defined there is : ".": { "import": "./dist/esm/index.mjs", "default": "./index.js" }

So, when you do import whatever-you-want-to -import from "caf", this comes from ./dist/esm/index.mjs, which looks like this : export{default as CAF}from"./caf.mjs";export{default as CAG}from"./cag.mjs";export{default as CAFShared}from"./shared.mjs";

This means you there is no default export in this file to use and as far as I understand it - you should import like this import { CAF } from "caf". On other hand import CAF from "caf/caf" would come from ./dist/esm/caf.mjs where the module.exports you mention happens.

lllopo avatar Jan 31 '22 15:01 lllopo

If you look at CAF's package.json, the default export defined there is :

I see this is something I missed. I only looked at main and I didn't see exports below. Thanks!

Well I'd gladly use import CAF from 'caf/caf'; or import { CAF } from 'caf';

But neither work for me here.

After a little bit of googling, it seems like it's a TypeScript problem:

https://github.com/microsoft/TypeScript/issues/33079

MartinMalinda avatar Feb 11 '22 07:02 MartinMalinda

So it seems like the situation is

Webpack 4 - does not support exports in package.json ❌ TypeScript - does not support exports in package.json ❌ Webpack 5 - does support exports in package.json ✅

I'm not sure 100% but it seems like as long as TS is used in the library the situation is quite blocked.

MartinMalinda avatar Feb 17 '22 11:02 MartinMalinda

Could this perhaps be temporarily resolved on WP5 side via resolve.alias ?

module.exports = {
  //...
  resolve: {
    alias: {
      caf: path.resolve(__dirname, 'node_modules/adjusted/path/here'),
    },
  },
};

MartinMalinda avatar Feb 17 '22 11:02 MartinMalinda

Could this perhaps be temporarily resolved on WP5 side via resolve.alias ?

module.exports = {
  //...
  resolve: {
    alias: {
      caf: path.resolve(__dirname, 'node_modules/adjusted/path/here'),
    },
  },
};

Pretty busy, but I'll give it a try when I have the time and let you know if it worked. I'm not putting too many hopes on it, as I think I tried something similar already and it didn't help. Still - I'll try again. A side note - Isn't it there something similar as solution for tsconfig then, so this can be sorted out universally at your side ?

lllopo avatar Feb 17 '22 14:02 lllopo

Sooooo ... I'm back. Did a few experiments and here the results.

First - your suggestion works like this : resolve: { alias: { caf: path.resolve(__dirname, 'node_modules/vue-concurrency/node_modules/caf/dist/esm/caf.mjs'), } }

but the problem is that this way I would have to do the old style imports of caf in my project, if I need it elsewhere. So, I find it somewhat unacceptable. For now I'll maybe use it, so I can use the dist of your project instead of re-building it, but I don't find it good practice. I think it would be much better solution to have this sorted out at vue-concurrency project level. So, what I found is that TypeScript configuration supports this :

"compilerOptions": { "baseUrl": ".", "paths" : { "..." : ["..."] } }

Wouldn't this somehow work, along with importing import CAF from 'caf/caf';.

lllopo avatar Feb 18 '22 09:02 lllopo