Skip to content

fix: Remove dicer dependency to libs/utils.js #102

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

Jump to bottom
Closed
lahirumaramba wants to merge 1 commit into from
Closed

fix: Remove dicer dependency to libs/utils.js #102

lahirumaramba wants to merge 1 commit into from

Conversation

@lahirumaramba
Copy link
Contributor

@lahirumaramba lahirumaramba commented Nov 21, 2022

App Services in Atlas Triggers does not support util.TextDecoder. As a result using firebase-admin (with Dicer through@fastify/busboy) on Atlas Triggers throws the errors reported in #100 and firebase/firebase-admin-node#1902

Looking at the code, it doesn't look like Dicer has a direct dependency on TextDecoder. IIUC the dependency to TextDecoder comes from libs/utils, which is required in deps/dicer/lib/HeaderParser.js to access the util function getLimit().

This PR removes the dependency to lib/utils from deps/dicer/lib/HeaderParser.js in turn removing the dependency to TextDecoder.

I simply copied the getLimit() to HeaderParser.js, but a better alternative would be to move getLimit() to it's own util file and import from there... that way we can keep a single implementation with single set of tests. It also seemed a bit of an overkill so I am proposing this first to see what you think. :)

[x] npm run test
[x] npm run bench:dicer

1612.90 mb/sec
1562.50 mb/sec
1470.59 mb/sec
1538.46 mb/sec
1219.51 mb/sec
1369.86 mb/sec
1333.33 mb/sec
1265.82 mb/sec
1351.35 mb/sec
1265.82 mb/sec

Resolves: #100

@lahirumaramba lahirumaramba mentioned this pull request Nov 21, 2022
Closed
Eomm
Eomm reviewed Nov 22, 2022
View reviewed changes
Copy link
Member

@Eomm Eomm left a comment

Choose a reason for hiding this comment

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

I read the issues, but it is not totally clear to me how this duplication solves the issue.

Is there any bundling process or mocking that breaks the import/require?

deps/dicer/lib/HeaderParser.js
}
}

function getLimit (limits, name, defaultLimit) {
Copy link
Member

@Eomm Eomm Nov 22, 2022

Choose a reason for hiding this comment

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

Suggested change
function getLimit (limits, name, defaultLimit) {
// This function is duplicated
// Read here for details https://github.com/firebase/firebase-admin-node/issues/1902
function getLimit (limits, name, defaultLimit) {

@kibertoad
Copy link
Member

kibertoad commented Nov 22, 2022

@Eomm TextDecoder is not supported by AtlasDB JS runtime.

@Eomm
Copy link
Member

Eomm commented Nov 22, 2022 •
edited
Loading

Ok, I got it, busboy/lib/utils.js contains a bad require

So a try/catch there should solve as well

@climba03003
Copy link
Member

climba03003 commented Nov 22, 2022

TextEncoder should be optional. Does it means that the whole util is not present?

busboy/lib/utils.js

Lines 110 to 120 in 05c990c

try {
textDecoders.set(destEncoding, new TextDecoder(destEncoding))
return textDecoders.get(destEncoding).decode(Buffer.from(text, textEncoding))
} catch (e) {
if (getEncoding(destEncoding)) {
try {
textDecoders.set(destEncoding, new PolyfillTextDecoder(destEncoding))
return textDecoders.get(destEncoding).decode(Buffer.from(text, textEncoding))
} catch (e) { }
}
}

@lahirumaramba
Copy link
Contributor Author

lahirumaramba commented Nov 28, 2022

Ah I think a try/catch is definitely a better way to address this. I will make the changes. Thanks for the feedback folks!

@itwseood
Copy link

itwseood commented Jan 5, 2023

Hi team. I am using MongoDB Atlas Serverless functions and following the thread reached this git. Thanks to @lahirumaramba for the fix! I see that the merging is blocked, though. What is the proper way to push the merge, so it reflects on the next library update? Sorry for the lame question.

@Eomm
Copy link
Member

Eomm commented Jan 5, 2023

Hi team. I am using MongoDB Atlas Serverless functions and following the thread reached this git. Thanks to @lahirumaramba for the fix! I see that the merging is blocked, though. What is the proper way to push the merge, so it reflects on the next library update? Sorry for the lame question.

Instead of copy-pasting the function, we need to add a try/catch (here details #102 (comment) )

Would you like to send a PR and speed up the process?

@Uzlopak Uzlopak mentioned this pull request Jan 5, 2023
Merged
@Uzlopak
Copy link
Contributor

Uzlopak commented Jan 5, 2023

Created a new PR #105

@Uzlopak Uzlopak closed this Jan 5, 2023
@lahirumaramba lahirumaramba deleted the lm-fix-textdecoder branch January 5, 2023 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Eomm Eomm Eomm left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

'node_modules/@fastify/busboy/lib/utils.js': TypeError: Value is not an object: undefined

6 participants

@lahirumaramba @kibertoad @Eomm @climba03003 @itwseood @Uzlopak