-
Notifications
You must be signed in to change notification settings - Fork 15
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
Webpack 5 fails due to the way CAF is imported #58
Comments
And also - is the project still alive ? |
Hey @lllopo a minimal reproduction repo would be very useful here 🙏 I'll try to see if |
I'm trying 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. |
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" :-). |
@MartinMalinda
works in my setup with Webpack 5. So if it works on yours too (supposedly it should), I'd rather say go for it. |
Thanks @lllopo . I'll check if |
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 I'm using latest Vue 3. |
@lllopo I couldn't fix this in But I released Once I can figure out the build issue or maybe once I migrate from microbundle to |
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. |
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. |
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). |
|
Ahhh ... bad. Maybe some kind of polyfill could solve this ? |
Here's what the ESM distribution of CAF looks like:
export {default as CAF} from "./caf.mjs";
export {default as CAG} from "./cag.mjs";
export {default as CAFShared} from "./shared.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 { 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 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 IOW, I'm not sure if WP5 is having trouble with the Does that make sense? |
This :
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 Edit 2: Note that |
I'm curious why you have to use such an explicit file path in the If at all possible, 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. |
Side note: I am considering changing that |
I'm not that good at it at all, so most of the things I mention are something of a guess. |
As of why not |
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"
No, that's not what you want. You should pick one of these two: import CAF from "caf/caf"
// or:
import { CAF } from "caf"
|
I still don't completely understand how export default Object.assign(CAF,{
cancelToken:cancelToken,
delay:delay,
timeout:timeout,
signalRace:signalRace,
signalAll:signalAll,
tokenCycle:tokenCycle
}); Here vue-c: imports it like this: And then uses like this: Therefore the default import is calleable and it works. And it makes sense, no? CAF is a function:
Object.assign(CAF,{
cancelToken:cancelToken,
delay:delay,
timeout:timeout,
signalRace:signalRace,
signalAll:signalAll,
tokenCycle:tokenCycle
}); What's the reason for |
@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). So, when you do 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 |
I see this is something I missed. I only looked at Well I'd gladly use But neither work for me here. After a little bit of googling, it seems like it's a TypeScript problem: |
So it seems like the situation is Webpack 4 - does not support I'm not sure 100% but it seems like as long as TS is used in the library the situation is quite blocked. |
Could this perhaps be temporarily resolved on WP5 side via 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. |
Sooooo ... I'm back. Did a few experiments and here the results. First - your suggestion works like this : 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 :
Wouldn't this somehow work, along with importing |
@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! |
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. |
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:
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 |
I tried setting a TS alias and with it I can get it to build it with 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 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. |
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. |
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
Only thing I can think of is to have the fallback more "future compatible" . If the consumer does not support https://github.com/getify/CAF/blob/master/package.json#L5 If main would be compatible with vue-concurrency points I'm not sure if that would be suitable for CAF. There's also a 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). |
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
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",
...
}
} |
Hoping maybe this is news that leads to a resolution here: https://devblogs.microsoft.com/typescript/announcing-typescript-4-7-beta/ |
Thanks for the heads up @getify 🙏 So far I'm trying
Together with 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
|
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 ( |
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 ? |
Initially came here because |
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. |
Hello @MartinMalinda, I've had another look at owncloud/web#7141 today and apparently using version I'm slightly confused by the 3.x and 4.x releases.
Is there a reason those are prereleases? Any chance to see a final release any time soon? Thanks for your time and efforts! |
Since #96 was merged, can anyone confirm it's alright in Webpack now? 🙏 |
Is this issue resolved now? |
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. |
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.
The text was updated successfully, but these errors were encountered: