MartinMalinda / vue-concurrency

A library for encapsulating asynchronous operations and managing concurrency for Vue and Composition API.
https://vue-concurrency.netlify.app/
MIT License
352 stars 15 forks source link

Webpack 5 fails due to the way CAF is imported #58

Closed lllopo closed 9 months ago

lllopo commented 2 years ago

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 commented 2 years ago

And also - is the project still alive ?

MartinMalinda commented 2 years ago

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 commented 2 years ago

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.

lllopo commented 2 years ago

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 commented 2 years ago

@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.

MartinMalinda commented 2 years ago

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

MartinMalinda commented 2 years ago

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.

lllopo commented 2 years ago

@MartinMalinda I'm using latest Vue 3.

MartinMalinda commented 2 years ago

@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.

lllopo commented 2 years ago

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.

MartinMalinda commented 2 years ago

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.

lllopo commented 2 years ago

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).

MartinMalinda commented 2 years ago

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

MartinMalinda commented 2 years ago

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

lllopo commented 2 years ago

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

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

getify commented 2 years ago

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?

lllopo commented 2 years ago

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.

getify commented 2 years ago

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 commented 2 years ago

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.

lllopo commented 2 years ago

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 commented 2 years ago

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.

vdvibhu20 commented 2 years ago

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

getify commented 2 years ago

@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.

MartinMalinda commented 2 years ago

@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.

lllopo commented 2 years ago

@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.

MartinMalinda commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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'),
    },
  },
};
lllopo commented 2 years ago

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 commented 2 years ago

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';.

MartinMalinda commented 2 years ago

@lllopo thanks for testing it out. I'll try to test the TS option soon. Hopefully the TS config will solve it for everyone and not just flip the issue to the other side (fixing it for WP5 and breaking it for others). Will check!

MartinMalinda commented 2 years ago

There's also an option to pass an alias to microbundle, the tool that actually builds the output bundle. Maybe that's the correct way to go around this. I'll try to check that also.

getify commented 2 years ago

Just as a side note: I am about to release (probably today or tomorrow) v15.0.0 of CAF. This version is important for the following reasons:

  1. It fixes currently broken behavior in most new browsers, due to changes the HTML spec made to AbortSignal that started to land in browsers recently (last few months).

  2. I am deprecating "caf/caf" as an import specifier, and will eventually remove it. I've long disliked it as being confusing. The new equivalent import specifier introduced in 15.0.0 is "caf/core".

There is already a pre-release of these updates on npm: 15.0.0-preA. I would encourage you to try that version out, and switch from "caf/caf" to "caf/core" to be future safe.

MartinMalinda commented 2 years ago

I tried setting a TS alias and with it I can get it to build it with import { CAF } from 'caf'. But I'm looking at the dist output and I'm thinking I'm just moving the issue upstream. With the TS alias I can bypass the problem in my own TS codebase. But if the consumer of vue-concurrency has TS won't it just break there? Same story with WP4, it's probably not gonna work there.

So it's a dilemma between shipping a "wrong" import that will probably work for majority of consumers as opposed to import that is "correct" but would work for a minority of consumers.

@getify thank you for this notice. Regarding the import issues I'm coming to the conclusion that it might be too early for library to use the package.exports maybe?. If someone uses CAF directly it's not a big deal as they can just use the "wrong" import if their environment does not support it. But it puts vue-concurrency in a weird spot as I can't just make it work for everyone.

I'm experimenting with this solution:

import * as CAF from "caf";

 // because not all environemnts support package.exports field (TS, WP4 and others), it's necessary to look for CAF function in two places
const _caf = CAF.CAF || CAF;
const token = new (CAF as any).cancelToken();
const cancelable = _caf(cb, token);

But I'm not sure if it actually does the trick. I'll have to test in different environments later.

getify commented 2 years ago

Regarding the import issues I'm coming to the conclusion that it might be too early for library to use the package.exports maybe?

If I understand this question, are you saying that a library like CAF should avoid defining package.exports stuff because the rest of the ecosystem seems to not understand how to use them?

That's a tough position to be in. From my perspective, I want to publish a package that's as easy as possible for as many people as possible to use. It's particularly ironic that in my attempt to provide these friendly import paths, and moreover to provide both named and default imports, which should have provided a wider umbrella of usage, seems to (at least in part) be limiting some folks. :(

If, for example, I did no ESM at all, and stuck with only CJS, then those who are really into ESM-only would be limited. If I did ESM only, it leaves behind people who are not yet on the ESM wagon, for a variety of reasons.

If I do ESM but don't include those package-exports, then people cannot use independent parts of CAF as easily, and to do so they always have to express full deep paths. There's not that many independent pieces in CAF for now, but I have other libraries with a dozen or more exports, so I want a strategy that "grows" with my library.

My perspective is, if someone came to me and said, "hey, if you include this XYZ file or this XYZ entry in your package.json, it would really help this or that tool work correctly", I'd be all for it. I am open and interested in having CAF be as useful to folks as I can.

But I think it would be a net-negative for CAF and its users if I removed the package-exports and broke all the folks who are using them fine, and forced them to go back to explicit paths. That's a pretty tough pill to swallow, especially if folks in those other tools realize they have catch-up still to play and are working on addressing it.

The challenge I have is, I don't personally use TS or Webpack, so I don't really have any insights into how they work, or should work. I'm relying on folks like those here who are in the weeds with it. I wish I had some better brilliant ideas.

But I think what I designed for CAF seems like the correct approach, even though it's clearly causing some issues in the ecosystem.

I dunno how to fix the situation. But if we can find something I could add, that wouldn't break how CAF is already used in other scenarios, I am perfectly happy to do that.

MartinMalinda commented 2 years ago

If I understand this question, are you saying that a library like CAF should avoid defining package.exports stuff because the rest of the ecosystem seems to not understand how to use them?

I'm sorry if that sounded rude from my side. I definitely don't want to instruct others on what they should do. I understand your points and there's a nice value in package.exports and hopefully in the future it will be a nice and easy way to check and define what a library exports.

I dunno how to fix the situation. But if we can find something I could add, that wouldn't break how CAF is already used in other scenarios, I am perfectly happy to do that.

Only thing I can think of is to have the fallback more "future compatible" . If the consumer does not support package.exports their bundler will most likely look at main and continue from there. In this case it lands at main:

https://github.com/getify/CAF/blob/master/package.json#L5

If main would be compatible with . export the import { CAF } from 'caf'; would work. But in this case main has a default export of CAF and therefore I'm so far doing import CAF from 'caf'.

vue-concurrency points main to dist: https://github.com/MartinMalinda/vue-concurrency/blob/master/package.json#L7

I'm not sure if that would be suitable for CAF.

There's also a module field but I'm not aware how much and by what consumers it is used: https://github.com/MartinMalinda/vue-concurrency/blob/master/package.json#L6 But maybe it could also function as a reasonable fallback if package.exports is not supported. I'm not sure :/

I'll see if I have time over the weekend, I might try these in CAF repository and report how it works with TypeScript 🙏 (as I'm using it).

twitwi commented 2 years ago

Working on a typescript, vite-based, vue3 project using vue-concurrency, I have a "require is not defined" error on importing caf (onyl) when running a built version (run build works but the hosting the dist raises the error).

A quick workaround I tried is monkey patching node_modules/vue-concurrency/dist/vue3/vue-concurrency.js

Here is the possibly relevant parts of the package.json

{
  "name": "...",
  "version": "0.0.0",
  "scripts": {
    "dev": "vite",
    "build": "vue-tsc --noEmit && vite build",
    ...
  },
  "dependencies": {
    "vue": "^3.2.31",
    "vue-concurrency": "^2.3.0",
    ...
  },
  "devDependencies": {
    "@vitejs/plugin-vue": "^2.2.2",
    "@vue/tsconfig": "^0.1.3",
    "typescript": "~4.5.5",
    "vite": "^2.8.4",
    "vue-tsc": "^0.31.4",
    ...
  }
}
getify commented 2 years ago

Hoping maybe this is news that leads to a resolution here: https://devblogs.microsoft.com/typescript/announcing-typescript-4-7-beta/

MartinMalinda commented 2 years ago

Thanks for the heads up @getify 🙏

So far I'm trying

 "typescript": "^4.7.0-beta",

Together with import CAF from "caf/core"; with no luck though.

Screenshot 2022-04-22 at 12 14 32

I've also tried to change the package.json of CAF in the node modules folder to this format (so it is in the same format as in the TS announcement example) and that also didnt solve the problem

    "exports": {
        ".": {
                "import": {
                         "default": "./dist/esm/index.mjs"
                     }
        },
derekwsgray commented 2 years ago

Working on a typescript, vite-based, vue3 project using vue-concurrency, I have a "require is not defined" error on importing caf (onyl) when running a built version (run build works but the hosting the dist raises the error).

Same situation, which led me here. Vite+Vue3, but no typescript. Didn't realize anything was wrong because it works in dev, just not in production (vite build, vite preview).

derekwsgray commented 2 years ago

But! I just tried 4.0.0-7 and the problem vanished 🎉

dschmidt commented 2 years ago

But! I just tried 4.0.0-7 and the problem vanished :tada:

Sorry, version 4.0.0-7 of what exactly?

pascalwengerter commented 2 years ago

But! I just tried 4.0.0-7 and the problem vanished 🎉

Sorry, version 4.0.0-7 of what exactly?

Perhaps https://github.com/MartinMalinda/vue-concurrency/releases/tag/4.0.0-7 ?

pascalwengerter commented 2 years ago

Initially came here because v2.3.0 doesn't work with Rollup either (https://github.com/owncloud/web/pull/7141 leads to broken unit tests complaining about caf/caf, don't let yourself be fooled by unrelated the CI error 😉). Any chance we get a v2.3.1 or an official vue2-compatible v3.0.0 anytime soon? :) thx!

MartinMalinda commented 2 years ago

Sorry for the delay everybody. I'll double check the situation again this week, hopefully a new compromise has appeared and if not, I'll release different versions for different platforms and let you know.

dschmidt commented 2 years ago

Hello @MartinMalinda,

I've had another look at https://github.com/owncloud/web/pull/7141 today and apparently using version 3.0.0-7 works just fine for us (with rollup btw).

I'm slightly confused by the 3.x and 4.x releases. Here's what I seem to understand:

Is there a reason those are prereleases? Any chance to see a final release any time soon?

Thanks for your time and efforts!

MartinMalinda commented 11 months ago

Since https://github.com/MartinMalinda/vue-concurrency/pull/96 was merged, can anyone confirm it's alright in Webpack now? 🙏

getify commented 10 months ago

Is this issue resolved now?

MartinMalinda commented 9 months ago

I'm giving this a quick test.

It seems like basic import works in Webpack 5, but then it errors out on a polyfill import.

https://codesandbox.io/p/sandbox/webpack5-forked-t5pzzn

But it looks its a codesandbox specific problem because in standblitz its all fine.

https://stackblitz.com/edit/webpack-webpack-js-org-tfxmcl?file=src%2Findex.js,package.json

Closing!

Thanks @Hawxy for the fix.