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.

Sign up for GitHub

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

throw with a clearer exception if using node-only function in browser #49

Closed
wants to merge 1 commit into from
Closed

throw with a clearer exception if using node-only function in browser #49

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 22 additions & 9 deletions src/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,21 +87,34 @@ export async function asyncBufferFromUrl({ url, byteLength, requestInit }) {
/**
* Construct an AsyncBuffer for a local file using node fs package.
*
* The function is not supported in the browser.
*
* @param {string} filename
* @returns {Promise<AsyncBuffer>}
*/
export async function asyncBufferFromFile(filename) {
const fsPackage = 'fs' // webpack no include
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this PR missed the reason for the "webpack no include" hack. If you put await import('fs') then webpack looks at it and says "oh it looks like I need to bundle this too" but then fails because fs is a node system package not available in npm or the browser.

So the problem is not exception handling, it's the bundling.

I think that the solution is to have a conditional export. Possible that asyncBufferFromFile needs to be in its own file to avoid aggressive bundling, but not sure.
https://nodejs.org/api/packages.html#conditional-exports

I'll see if I can get a repro on the bundling issue. I think it happened when I imported from hyperparam or the cli without the string hack.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohhhh OK. I'll close this PR, and maybe look at this issue later, as it's not super urgent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in the PR message, I'm not sure if conditional exports is the solution because a Node.js app can use either ESM imports or CJS require.

const fs = await import(fsPackage)
const stat = await fs.promises.stat(filename)
return {
byteLength: stat.size,
async slice(start, end) {
try {
const fs = await import('fs')
const stat = await fs.promises.stat(filename)
return {
byteLength: stat.size,
async slice(start, end) {
// read file slice
const readStream = fs.createReadStream(filename, { start, end })
return await readStreamToArrayBuffer(readStream)
},
const readStream = fs.createReadStream(filename, { start, end })
return await readStreamToArrayBuffer(readStream)
},
}
} catch (e) {
if (e instanceof TypeError) {
/// In the browser, the following error is thrown:
/// TypeError: The specifier “fs” was a bare specifier, but was not remapped to anything.
/// Relative module specifiers must start with “./”, “../” or “/”.
throw new Error('asyncBufferFromFile is not supported in browser')
} else {
throw e // re-throw the error unchanged
}
}

}

/**
Expand Down
Loading