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

Cannot import processResponsePayload from webdav/dist/node/response.js #339

Closed
skjnldsv opened this issue Mar 31, 2023 · 18 comments · Fixed by #345
Closed

Cannot import processResponsePayload from webdav/dist/node/response.js #339

skjnldsv opened this issue Mar 31, 2023 · 18 comments · Fixed by #345

Comments

@skjnldsv
Copy link
Contributor

Something changed recently on the package export, and we used to be able to import it to manually do a custom request and parse the content ourselves.
I'm guessing since we're building for web, the exports is what makes our webpack process not find it?
Is there any way to mitigate this @perry-mitchell ? :)

You can see the (ugly) way around we had to do here:
https://github.com/nextcloud/server/blob/2afe26726bb48f78ef80ef1549834160121caaa4/apps/comments/src/services/GetComments.js

Cheers! ✌️

@perry-mitchell
Copy link
Owner

Hi @skjnldsv - So sorry I didn't get back to you sooner.. A full quarter later. If you're still having this issue I'd be more than happy to make the necessary modifications for you to be able to continue making custom requests. Let me look into this for you.

@perry-mitchell
Copy link
Owner

Regarding these lines: https://github.com/nextcloud/server/blob/2afe26726bb48f78ef80ef1549834160121caaa4/apps/comments/src/services/GetComments.js#-L63-L64

You probably don't need processResponsePayload in this case because it merely wraps the data in the response when required, that's all: https://github.com/perry-mitchell/webdav-client/blob/master/source/response.ts#L39-L46

That being said, I'm going to expose it and release a new version so you can continue to use it as-is. Hopefully that's enough - please let me know if you require something additional.

@perry-mitchell
Copy link
Owner

Released under 5.2.1

@skjnldsv
Copy link
Contributor Author

Thank you!! :)

@skjnldsv
Copy link
Contributor Author

Ah, it seems t still misbehave

Module not found: Error: Package path ./dist/node/request is not exported from package /home/admin/Docker/server/node_modules/webdav (see exports field in /home/admin/Docker/server/node_modules/webdav/package.json)

@skjnldsv
Copy link
Contributor Author

Fix in #345

@perry-mitchell
Copy link
Owner

perry-mitchell commented Jun 26, 2023

Might I ask though, why you're importing from the node folder when using the browser definition in package.json? That.. doesn't make sense 😄

If you just import from "webdav" it should resolve the best import for the environment.. Not sure why in your MR you're importing from node.

I'll hold the release until I hear back sorry - want to make sure there's not some misunderstanding here :)

@skjnldsv
Copy link
Contributor Author

We're bundling everything though webpack, so browser or node should not matter that much I think :)
You would suggest we import everything from /dist/node then?

Like on this specific example above, how could I replace the request method without importing from the node folder ?

@perry-mitchell
Copy link
Owner

Ok.. interesting. I'm not sure how Nextcloud server works under the hood, but it seems strange that you're using Webpack to build it when it's supposedly run on Node? Why not just build it using Typescript?

Webpack in this case is trying to resolve via the browser manifest in package.json, which is wrong, if you're building a node app. If it's web it's fine and then you could just import from "webdav". Node should be imported from the same and when the environment is correct, it will resolve using "exports" or "main" in package.json.

Any chance that you don't have "target" set in your Webpack config? I wonder if, when set to "node", if this would change how Webpack resolves package.json dependencies 🤔.

Apologies if I've misunderstood something/everything.. the setup seems strange to me, that's all :)

@skjnldsv
Copy link
Contributor Author

The setup is very standard actually,
We're bundling lots of libraries and our code to work on specific browsers.
Webpack is commonly used like that

(We also have babel transpiling any code that would be incompatible with the range of browsers we desire)

https://webpack.js.org/guides/package-exports/#target-environment

@skjnldsv
Copy link
Contributor Author

Btw, generally speaking, there is fewer and fewer people running libraries straight from their browsers.
Most are using pre-processors, like vite, or webpack.

We're maintaining a fair amount of libs at Nextcloud, and we only export esm and cjs packages.
People that want to run them in their browsers can use those tools to built them in their code.

It's far simpler to consider an npm dependency as a library only instead of a runtime/library combo in my honest opinion :)
https://github.com/nextcloud/nextcloud-l10n/blob/7360cce65fa8bb050f26ff89319a28cafd9b08de/package.json#L5-L17

@perry-mitchell
Copy link
Owner

perry-mitchell commented Jun 27, 2023

We're bundling lots of libraries and our code to work on specific browsers.

Yes, but the original code linked was for nextcloud server, which doesn't run in the browser, right? Ie here:

You can see the (ugly) way around we had to do here:
https://github.com/nextcloud/server/blob/2afe26726bb48f78ef80ef1549834160121caaa4/apps/comments/src/services/GetComments.js

Most are using pre-processors, like vite, or webpack.

I'm aware :) - I use webpack liberally, but not for server-side libraries. I just compile ts => js using typescript (tsc) then.

So.. am I right in assuming that the repo is a monolith of projects and you just use webpack to simplify the building of all of them?

If so, then I can see the import being a problem. Would it work for you if I removed the other paths in browser and just added ./dist so all of dist is accessible? Not sure if that's how browser works though.

@skjnldsv
Copy link
Contributor Author

Yes, but the original code linked was for nextcloud server, which doesn't run in the browser, right? Ie here:

Oh, it's misleading, my bad, yes; server includes both the backend and the frontend :)
It's the entire base infrastructure, to which apps can/do get installed to extend functionalities

So.. am I right in assuming that the repo is a monolith of projects and you just use webpack to simplify the building of all of them?

Kind of yeah :)

If so, then I can see the import being a problem. Would it work for you if I removed the other paths in browser and just added ./dist so all of dist is accessible? Not sure if that's how browser works though.

I have never seen exports like you did here, and I'm lacking expertise on the subject.
The only exports we do and are using is to create cjs/esm hybrid libraries that are only meant to be included by a pre-processor.
In which case the exports looks like that (recent accepted consensus on the matter, 2021-2022 iirc)

  "main": "dist/index.js",
  "types": "dist/index.d.ts",
  "exports": {
    ".": {
      "types": "./dist/index.d.ts",
      "import": "./dist/index.mjs",
      "require": "./dist/index.js"
    }
  },

Some do decide to go esm only, like this famous developer with many used libs: https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c

@perry-mitchell
Copy link
Owner

Yeah. To be honest these hybrid outputs like webdav is using are painful, unstable and just a lot of work. I'd rather go full ESM but I don't think it's appropriate.. yet.

In your example now you show only exports.. I can find tonnes of documentation on that but very little on browser.. I'd like to remove it but it seems like some people require it. Or I remember that some users had pointed out how other libraries have done it, like here on my ulidx library. I would like to remove it, but I don't know what effect that will have. I'll probably just release your fix and then in the next major version drop support for browser. exports is the accepted modern way that should soon support all use cases.

@skjnldsv
Copy link
Contributor Author

I'll probably just release your fix and then in the next major version drop support for browser. exports is the accepted modern way that should soon support all use cases.

I think you're right and this is the safest way :)

@skjnldsv
Copy link
Contributor Author

@perry-mitchell could you release v5.2.2 ? :)

@perry-mitchell
Copy link
Owner

Released :)

@skjnldsv
Copy link
Contributor Author

You rock!! 🎸

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants