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

replace into-stream with Readable.from #289

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 4 additions & 4 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ const fp = require('fastify-plugin')
const encodingNegotiator = require('@fastify/accept-negotiator')
const pump = require('pump')
const mimedb = require('mime-db')
const intoStream = require('into-stream')
const { Readable } = require('readable-stream')
Copy link
Member

@climba03003 climba03003 Mar 29, 2024

Choose a reason for hiding this comment

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

Side note, should install readable-stream once and update the package.json dependencies.

const peek = require('peek-stream')
const { Minipass } = require('minipass')
const pumpify = require('pumpify')
Expand Down Expand Up @@ -273,7 +273,7 @@ function buildRouteCompress (fastify, params, routeOptions, decorateOnly) {
if (Buffer.byteLength(payload) < params.threshold) {
return next()
}
payload = intoStream(payload)
payload = Readable.from(payload)
}

params.removeContentLengthHeader
Expand Down Expand Up @@ -398,7 +398,7 @@ function compress (params) {
if (Buffer.byteLength(payload) < params.threshold) {
return this.send(payload)
}
payload = intoStream(payload)
payload = Readable.from(payload)
}

params.removeContentLengthHeader
Expand Down Expand Up @@ -506,7 +506,7 @@ function maybeUnzip (payload, serialize) {
// handle case where serialize doesn't return a string or Buffer
if (!Buffer.isBuffer(buf)) return result
if (isCompressed(buf) === 0) return result
return intoStream(result)
return Readable.from(result)
}

function zipStream (deflate, encoding) {
Expand Down
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
"dependencies": {
"@fastify/accept-negotiator": "^1.1.0",
"fastify-plugin": "^4.5.0",
"into-stream": "^6.0.0",
"mime-db": "^1.52.0",
"minipass": "^7.0.2",
"peek-stream": "^1.1.3",
Expand All @@ -26,7 +25,8 @@
"standard": "^17.1.0",
"tap": "^16.3.7",
"tsd": "^0.30.0",
"typescript": "^5.1.6"
"typescript": "^5.1.6",
"undici": "^5.28.3"
},
"scripts": {
"coverage": "npm run test:unit -- --coverage-report=html",
Expand Down
40 changes: 40 additions & 0 deletions test/issue-288.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
'use strict'

const { test } = require('tap')
const Fastify = require('fastify')
const fastifyCompress = require('..')
const fsPromises = require('fs').promises
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const fsPromises = require('fs').promises

const { join } = require('path')
const { fetch } = require('undici')

Copy link
Contributor

@stanleyxu2005 stanleyxu2005 Mar 29, 2024

Choose a reason for hiding this comment

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

Suggested change
const longStringWithEmoji = new Array(5_000)
.fill('0')
.map(() => {
const random = new Array(10).fill('A').join('🍃')
return random + '- FASTIFY COMPRESS,🍃 FASTIFY COMPRESS'
})
.join('\n')

test('should not corrupt the file content', async (t) => {
const fastify = new Fastify()
t.teardown(() => fastify.close())

fastify.register(async (instance, opts) => {
await fastify.register(fastifyCompress)
instance.get('/issue', async (req, reply) => {
const longStringWithEmoji = await fsPromises.readFile(join(__dirname, './test.txt'), 'utf-8')
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const longStringWithEmoji = await fsPromises.readFile(join(__dirname, './test.txt'), 'utf-8')


return longStringWithEmoji // <--- the file content is corrupt
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return longStringWithEmoji // <--- the file content is corrupt
return longStringWithEmoji

// search for "hydra.alibaba.com" will see 2 wired question marks instead of emoji
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// search for "hydra.alibaba.com" will see 2 wired question marks instead of emoji
// Expect response should not contain any �

})
})

fastify.get('/good', async (req, reply) => {
const longStringWithEmoji = await fsPromises.readFile(join(__dirname, './test.txt'), 'utf-8')
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const longStringWithEmoji = await fsPromises.readFile(join(__dirname, './test.txt'), 'utf-8')
const longStringWithEmoji = await fsPromises.readFile(join(__dirname, './test.txt'), 'utf-8')


return longStringWithEmoji // <--- the file content is ok
// search for "hydra.alibaba.com" will see emoji
Comment on lines +27 to +28
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return longStringWithEmoji // <--- the file content is ok
// search for "hydra.alibaba.com" will see emoji
return longStringWithEmoji
// Expect response should not contain any �

})

await fastify.listen({ port: 0 })

const { port } = fastify.server.address()
const url = `http://localhost:${port}`
const response = await fetch(`${url}/issue`)
const response2 = await fetch(`${url}/good`)
const body = await response.text()
const body2 = await response2.text()
t.equal(body, body2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
t.equal(body, body2)
t.equal(body, body2)
t.equal(body.indexOf('�'), -1)
t.equal(body2.indexOf('�'), -1)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure number of emoji is valid

})
Loading
Loading