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

Buffer is not defined #42

Closed
surilindur opened this issue Aug 30, 2022 · 16 comments
Closed

Buffer is not defined #42

surilindur opened this issue Aug 30, 2022 · 16 comments

Comments

@surilindur
Copy link
Contributor

When used in a browser without Node.js polyfills, the jsonparse library produces an error about Buffer not being defined.

The error comes from here, in the Parser function: https://github.com/creationix/jsonparse/blob/1.3.1/jsonparse.js#L59

But there are also other locations where the global is used.

The library exports the Parser function at the end, which is then imported here and called in two locations, producing the error:

So whenever the Parser function is called, it attempts to access the Buffer global by the looks of it.

Maybe that global could somehow be provided by hand, when there is no Webpack or other means to do it? Or maybe the Buffer could be directly replaced with Uint8Array if it does not use any methods from the Node subclass?

@rubensworks
Copy link
Owner

@Tpt Since you worked on this recently, do you have any additional insights about this issue?

@Tpt
Copy link
Contributor

Tpt commented Aug 30, 2022

Yes, the buffer package should be installed for the webpack build to work. readable-stream requires it to target browsers but does not explicitly requires it.

@rubensworks
Copy link
Owner

But buffer already is a dependency of this project, right?

@rubensworks
Copy link
Owner

@surilindur Can you check what happens if we add buffer as a direct dependency to https://github.com/comunica/jQuery-Widget.js ?

@Tpt
Copy link
Contributor

Tpt commented Aug 30, 2022

But buffer already is a dependency of this project, right?

Indeed, that's a good point. I missed that sorry. jsonparse seems to assume that Buffer is global. It's maybe not the case in @surilindur's project. But I don't find any place where Buffer is declared as global in this package CI Webpack build. So I am not sure what is working here that is not working in @surilindur's project. rdfxml-streaming-parser.js seems to have the same behavior.

@rubensworks
Copy link
Owner

But I don't find any place where Buffer is declared as global in this package CI Webpack build.

Perhaps the build process itself runs fine, but problems may only arise when using the webpacked code.

@Tpt Since you were working on a fork of jsonparse in https://github.com/comunica/json-event-parser.js, do you think that's in a usable enough state already to use in here as well?

If that's not the case yet, @surilindur I would suggest adding a polyfill for buffer in the webpack config for now until we find a solution to the problem.

@surilindur
Copy link
Contributor Author

Okay! Thank you both for input. I will see where and how to add it.

And yes, the problem seems to be the use of the global, without it being defined anywhere for the browser, and it only throws an error when actually running the code that tries to use it.

@Tpt
Copy link
Contributor

Tpt commented Aug 30, 2022

@Tpt Since you were working on a fork of jsonparse in https://github.com/comunica/json-event-parser.js, do you think that's in a usable enough state already to use in here as well?

Yes, it should work and bring real streaming. I managed to get the JSON-LD parser working on top of it (but I needed to keep the JSON content inside of the JSON-LD parser to keep all its behaviors working).

@rubensworks
Copy link
Owner

but I needed to keep the JSON content inside of the JSON-LD parser to keep all its behaviors working

Yeah, that's what I suspected. We may have to make some changes in the JSON-LD parser to not require this persistence of all JSON content, as we'll run into memory issues for long/infinite JSON-LD documents.

(Probably should discuss this elsewhere)

@Tpt
Copy link
Contributor

Tpt commented Aug 31, 2022

I have opened #43 that migrates to json-event-parser (good test for the library)

@rubensworks
Copy link
Owner

Very nice, thanks @Tpt!

(please remind me to review that PR once we release json-event-parser :-) )

@rubensworks
Copy link
Owner

@surilindur FYI, this PR was just merged: nodejs/readable-stream#489
Could you check if this resolves all of our problems, and if our workaround can be undone?

@surilindur
Copy link
Contributor Author

I tried to look at what the changes in the PR do, but it seems like it only defines the Buffer variable internally in the readable-stream library for the files that depend on it, and does not define it globally. I also tested it by trying to squeeze the latest version everywhere possible but it did not make a difference. 😦

So it does fix the need for polyfills, but only for readable-stream itself, not anything else that requires Buffer.

@rubensworks
Copy link
Owner

but it seems like it only defines the Buffer variable internally in the readable-stream library for the files that depend on it, and does not define it globally.

I guess that should be sufficient?

I also tested it by trying to squeeze the latest version everywhere possible but it did not make a difference.

Do you know which other libs still depend on buffer?

@surilindur
Copy link
Contributor Author

surilindur commented Oct 3, 2022

Actually, I tested with the wrong configuration, my bad, I completely forgot #43 and the changes there. With those changes, the workaround can be removed, as long as the new json-event-parser library uses the ^4.2.0 version of readable-stream. This is awesome. Apologies again. 😓

@rubensworks
Copy link
Owner

That's great news, thanks for checking @surilindur!

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