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

Browser import issue #290

Open
kylegerber94 opened this issue Aug 25, 2023 · 8 comments
Open

Browser import issue #290

kylegerber94 opened this issue Aug 25, 2023 · 8 comments

Comments

@kylegerber94
Copy link

kylegerber94 commented Aug 25, 2023

I'm trying to use MDBReader in the browser for a project that I'm serving with Web Dev Server, but keep hitting the following error:

Error while handling server request.
PluginError: Could not resolve import "crypto".

I've also tried directly referencing the built browser version

import { MDBReader } from 'mdb-reader/lib/browser/index.js';

but this yields a different error in lib/browser/environment/index.js where browserifyAES fails to import because it doesn't provide a default export.

Am I importing something incorrectly in either scenario?

@kylegerber94 kylegerber94 changed the title Could not resolve import "crypto" Browser import issue Aug 25, 2023
@andipaetzold
Copy link
Owner

👋

For the first error: Looks like the node version of mdb-reader is used instead of the browser version. With "Web Dev Server" are you referring to this: https://modern-web.dev/docs/dev-server/overview/? Could you provide me with instructions on how to reproduce your error - I am not familiar with that software. In theory the browser version should automatically used based on this value in the package.json: https://github.com/andipaetzold/mdb-reader/blob/main/package.json#L6

For the second error, it looks like other bundlers handle this issue gracefully: browserify-aes/browser.js doesn't have a default export: https://github.com/browserify/browserify-aes/blob/master/browser.js. I am just wondering whether me fixing my imports would fix that issue as there are more missing default exports within browserify-aes. Maybe you this can be changed by a config in Web Dev Server?

@tannerstern
Copy link

Hi Andi,

I work with Kyle. To explain a bit: yes, we're using @web/dev-server. The server uses a config file that works similarly to rollup's own config file and allows you to pass options into the same @rollup/plugin-node-resolve plugin used by rollup for resolving imports. I expect we are getting hung up there.

In our config we simply have nodeResolve: true. By default, this does not include the "browser" main field or export condition. If I add "browser" to those options, other packages in our app don't work, so we're limited here. Admittedly, our serving and building needs are quite complex due to a number of packages we rely on.

I say all this to say, it's probably-definitely something on our end, but our hands are tied.

@andipaetzold
Copy link
Owner

👋

nodeResolve: true somewhat means that things should be imported as if we were in node, right? So the mdb-reader seems to be configured correctly here as the node version is used. I can't change the package to serve the browser version in node environments. That could work but would reduce performance for all node users. parcel has a feature to polyfill node modules like crypto, maybe web dev server has something similar?

When I try using @web/dev-server with { browser: true } to enforce the use of browser in package.json, I still run into issues with fast-xml-reader: The requested module './../../../../../../../fast-xml-parser/src/fxp.js' does not provide an export named 'default'. The error is similar to the browserify-aes error above, so it seems like web dev server has more general issues with importing packages and needs to configured differently for default imports.

Besides adjusting the imports of browserify-aes and create-hash, there is probably nothing I can do without also breaking the package for other users. If you find any web dev server specific tweaks that are necessary, maybe patch-package can help?

@andipaetzold
Copy link
Owner

If you have an example of a package with node & browser version that works with your, please send it my way. Happy to take a look at their configuration and maybe adjust mine accordingly.

@tannerstern
Copy link

Andi,

I think you are exactly right. We've had issues with other packages with @web/dev-server which is why I suspected the issue was not with mdb-reader but was instead an issue with the server. Like you mentioned, it appears to handle default imports differently (at least differently from our rollup build which has never had any of these problems, though it leverages commonjs much more heavily).

Thank you for putting me on to patch-package, that looks like it could be very useful! At the moment though we've switched to using CDN for this dependency, which will work for now until we figure out how to tackle the issues with the server.

Because you asked, one package that we have never had any issues with using both in node and in the browser is the moment offshoot luxon. Surely there are architectural requirements that differentiate that package from yours (and that package has no "browser" import to begin with), but it works nicely both in Node and in the browser. Again, might be a completely unhelpful example given the differences, and this is certainly not to say anything about a right or wrong way to publish a package. Your package file works for many others (in the browser no less), it is only us who is having a problem.

Thanks again for a quick response! As far as I'm concerned, you've helped us more than enough already! We'll keep an eye on the issues list over on web-dev-serve and try to figure out what's up.

@andipaetzold
Copy link
Owner

If you come up with a change that makes mdb-reader work for you, feel free to open a PR. I'd love to have compatibility with all tooling people could user.

luxon: That package has mutliple versions for node, browser, esm etc. However, they are never using node-internal modules. The only difference between the packages is the build target and how things are bundled.

At some point I want to remove some dependencies from this package and only use crypto and window.crypto. That way I could get rid of these fairly old dependencies - unfortunately, I haven't found time to do so, yet. Not sure it's even possible.

@andipaetzold
Copy link
Owner

Hey @tannerstern,

I released a new (major) version of mdb-reader which includes conditional exports on the package.json. In my test, I don't have to use nodeResolve anymore. Maybe this also fixes some issues in your build setup.

@tannerstern
Copy link

Andi,

I got SO CLOSE to having this working with a local install (we are currently using the CDN+esm version). Your package.json changes did the trick and now the imports work out of the box! I did however have to handle browserify-aes and create-hash, and that's where I got hung up again.

Both of these gave X does not have a default export errors, which I solved by allowing @rollup/plugin-commonjs do the loading (and browserify-aes also needed @rollup/plugin-json to load a JSON file). However I hit a wall with create-hash:

Error while handling server request.
PluginError: Could not resolve import "crypto".

I suspect the reason I continue to have issues here is because we still have lingering issues preventing us from setting our Node resolve options to { browser: true }. I'll have to try again when we can.

Thanks anyway though! I got much further this time with your changes.

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

No branches or pull requests

3 participants