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

feat: Allow explicit null encoding for readFile/writeFile #18534

Merged
merged 16 commits into from
Oct 26, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion cli/types/cypress.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5677,7 +5677,7 @@ declare namespace Cypress {
xhr: XMLHttpRequest
}

type Encodings = 'ascii' | 'base64' | 'binary' | 'hex' | 'latin1' | 'utf8' | 'utf-8' | 'ucs2' | 'ucs-2' | 'utf16le' | 'utf-16le'
type Encodings = 'ascii' | 'base64' | 'binary' | 'hex' | 'latin1' | 'utf8' | 'utf-8' | 'ucs2' | 'ucs-2' | 'utf16le' | 'utf-16le' | null
type PositionType = 'topLeft' | 'top' | 'topRight' | 'left' | 'center' | 'right' | 'bottomLeft' | 'bottom' | 'bottomRight'
type ViewportPreset = 'macbook-16' | 'macbook-15' | 'macbook-13' | 'macbook-11' | 'ipad-2' | 'ipad-mini' | 'iphone-xr' | 'iphone-x' | 'iphone-6+' | 'iphone-se2' | 'iphone-8' | 'iphone-7' | 'iphone-6' | 'iphone-5' | 'iphone-4' | 'iphone-3' | 'samsung-s10' | 'samsung-note9'
interface Offset {
Expand Down
33 changes: 33 additions & 0 deletions packages/driver/cypress/integration/commands/files_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,22 @@ describe('src/cy/commands/files', () => {
})
})

// https://github.com/cypress-io/cypress/issues/1558
it('passes explicit null encoding through to server and decodes response', () => {
BlueWinds marked this conversation as resolved.
Show resolved Hide resolved
Cypress.backend.resolves({
contents: Buffer.from('\n'),
filePath: '/path/to/foo.json',
})

cy.readFile('foo.json', null).then(() => {
expect(Cypress.backend).to.be.calledWith(
'read:file',
'foo.json',
{ encoding: null },
)
}).should('eql', Buffer.from('\n'))
})

it('sets the contents as the subject', () => {
Cypress.backend.resolves(okResponse)

Expand Down Expand Up @@ -340,6 +356,23 @@ describe('src/cy/commands/files', () => {
})
})

// https://github.com/cypress-io/cypress/issues/1558
it('explicit null encoding is sent to server as Buffer', () => {
Cypress.backend.resolves(okResponse)

cy.writeFile('foo.txt', Buffer.from([0, 0, 54, 255]), null).then(() => {
expect(Cypress.backend).to.be.calledWith(
'write:file',
'foo.txt',
Buffer.from([0, 0, 54, 255]),
{
encoding: null,
flag: 'w',
},
)
})
})

it('can take encoding as part of options', () => {
Cypress.backend.resolves(okResponse)

Expand Down
11 changes: 11 additions & 0 deletions packages/driver/cypress/integration/commands/fixtures_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,17 @@ describe('src/cy/commands/fixtures', () => {
})
})

// https://github.com/cypress-io/cypress/issues/1558
it('passes explicit null encoding through to server and decodes response', () => {
BlueWinds marked this conversation as resolved.
Show resolved Hide resolved
Cypress.backend.withArgs('get:fixture').resolves(Buffer.from('\n'))

cy.fixture('foo', null).then((obj) => {
expect(Cypress.backend).to.be.calledWith('get:fixture', 'foo', {
encoding: null,
})
}).should('eql', Buffer.from('\n'))
})

it('can have encoding as second argument and options as third argument', () => {
Cypress.backend.withArgs('get:fixture').resolves({ foo: 'bar' })

Expand Down
28 changes: 23 additions & 5 deletions packages/driver/src/cy/commands/files.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,16 @@ export default (Commands, Cypress, cy) => {

if (_.isObject(encoding)) {
userOptions = encoding
encoding = null
encoding = undefined
BlueWinds marked this conversation as resolved.
Show resolved Hide resolved
}

options = _.defaults({}, userOptions, {
encoding: encoding != null ? encoding : 'utf8',
// https://github.com/cypress-io/cypress/issues/1558
// If no encoding is specified, then Cypress has historically defaulted
// to `utf8`, because of it's focus on text files. This is in contrast to
// NodeJs, which defaults to binary. We allow users to pass in `null`
// to restore the default node behavior.
encoding: encoding === undefined ? 'utf8' : encoding,
flotwig marked this conversation as resolved.
Show resolved Hide resolved
log: true,
})

Expand Down Expand Up @@ -53,6 +58,14 @@ export default (Commands, Cypress, cy) => {
args: { cmd: 'readFile', action: 'read', file, filePath: err.filePath, error: err.message },
})
}).then(({ contents, filePath }) => {
// https://github.com/cypress-io/cypress/issues/1558
// We invoke Buffer.from() in order to transform this from an ArrayBuffer -
// which socket.io uses to transfer the file over the websocket - into a
// `Buffer`, which webpack polyfills in the browser.
if (options.encoding === null) {
contents = Buffer.from(contents)
}

consoleProps['File Path'] = filePath
consoleProps['Contents'] = contents

Expand Down Expand Up @@ -85,11 +98,16 @@ export default (Commands, Cypress, cy) => {

if (_.isObject(encoding)) {
userOptions = encoding
encoding = null
encoding = undefined
}

options = _.defaults({}, userOptions, {
encoding: encoding ? encoding : 'utf8',
// https://github.com/cypress-io/cypress/issues/1558
// If no encoding is specified, then Cypress has historically defaulted
// to `utf8`, because of it's focus on text files. This is in contrast to
// NodeJs, which defaults to binary. We allow users to pass in `null`
// to restore the default node behavior.
encoding: encoding === undefined ? 'utf8' : encoding,
flag: userOptions.flag ? userOptions.flag : 'w',
log: true,
})
Expand Down Expand Up @@ -120,7 +138,7 @@ export default (Commands, Cypress, cy) => {
})
}

if (_.isObject(contents)) {
if (_.isObject(contents) && !Buffer.isBuffer(contents)) {
contents = JSON.stringify(contents, null, 2)
}

Expand Down
18 changes: 17 additions & 1 deletion packages/driver/src/cy/commands/fixtures.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ import Promise from 'bluebird'
import $errUtils from '../../cypress/error_utils'

const clone = (obj) => {
if (Buffer.isBuffer(obj)) {
return Buffer.from(obj)
}

return JSON.parse(JSON.stringify(obj))
}

Expand Down Expand Up @@ -47,7 +51,7 @@ export default (Commands, Cypress, cy, state, config) => {
options = args[1]
}

if (_.isString(args[0])) {
if (_.isString(args[0]) || args[0] === null) {
options.encoding = args[0]
}

Expand All @@ -64,6 +68,18 @@ export default (Commands, Cypress, cy, state, config) => {
return $errUtils.throwErr(response.__error)
}

// https://github.com/cypress-io/cypress/issues/1558
// We invoke Buffer.from() in order to transform this from an ArrayBuffer -
// which socket.io uses to transfer the file over the websocket - into a
// `Buffer`, which webpack polyfills in the browser.
if (options.encoding === null) {
response = Buffer.from(response)
} else if (response instanceof ArrayBuffer) {
// Cypress' behavior is to base64 encode binary files if the user
Copy link
Contributor Author

@BlueWinds BlueWinds Oct 25, 2021

Choose a reason for hiding this comment

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

@flotwig - As part of removing the server-side transform of buffers to base64 before transmitting (based on your review, which I 100% agree with), we now have to explicitly do so on the driver side unless a null encoding is specified, in order to maintain backwards compatibility.

In a vacuum it would make more conceptual sense to remove the default of utf8 for undefined encoding (and unknown file types) and base64 encoding for binary files, but that would turn this into a breaking change, so instead I ended up with this fairly messy set of conditionals spread around the driver in order to maintain backwards compatibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

In a vacuum it would make more conceptual sense to remove the default of utf8 for undefined encoding (and unknown file types) and base64 encoding for binary files, but that would turn this into a breaking change

Yeah, and I wonder if that breaking change would even be welcome. I would guess that many people are relying on the existing behavior that treats everything as utf8 by default.

// doesn't explicitly pass `null` as the encoding.
response = Buffer.from(response).toString('base64')
}

// add the fixture to the cache
// so it can just be returned next time
cache[fixture] = response
Expand Down
2 changes: 1 addition & 1 deletion packages/net-stubbing/lib/server/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ export async function setResponseFromFixture (getFixtureFn: GetFixtureFn, static
return
}

const data = await getFixtureFn(fixture.filePath, { encoding: fixture.encoding || null })
const data = await getFixtureFn(fixture.filePath, { encoding: fixture.encoding })

const { headers } = staticResponse

Expand Down
11 changes: 8 additions & 3 deletions packages/server/lib/files.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,14 @@ const { fs } = require('./util/fs')
module.exports = {
readFile (projectRoot, file, options = {}) {
const filePath = path.resolve(projectRoot, file)
const readFn = path.extname(filePath) === '.json' ? fs.readJsonAsync : fs.readFileAsync
const readFn = (path.extname(filePath) === '.json' && options.encoding !== null) ? fs.readJsonAsync : fs.readFileAsync

return readFn(filePath, options.encoding || 'utf8')
// https://github.com/cypress-io/cypress/issues/1558
// If no encoding is specified, then Cypress has historically defaulted
// to `utf8`, because of it's focus on text files. This is in contrast to
// NodeJs, which defaults to binary. We allow users to pass in `null`
// to restore the default node behavior.
return readFn(filePath, options.encoding === undefined ? 'utf8' : options.encoding)
.then((contents) => {
return {
contents,
Expand All @@ -22,7 +27,7 @@ module.exports = {
writeFile (projectRoot, file, contents, options = {}) {
const filePath = path.resolve(projectRoot, file)
const writeOptions = {
encoding: options.encoding || 'utf8',
encoding: options.encoding === undefined ? 'utf8' : options.encoding,
flag: options.flag || 'w',
}

Expand Down
16 changes: 12 additions & 4 deletions packages/server/lib/fixture.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
const _ = require('lodash')
const path = require('path')
const check = require('syntax-error')
const debug = require('debug')('cypress:server:fixture')
Expand Down Expand Up @@ -116,15 +115,24 @@ module.exports = {
},

parseFileByExtension (p, fixture, ext, options = {}) {
// https://github.com/cypress-io/cypress/issues/1558
// If the user explicitly specifies `null` as the encoding, we treat the
// file as binary regardless of extension. We base64 encode them for
// transmission over the websocket. There is a matching Buffer.from()
// in packages/driver/src/cy/commands/fixtures.ts
if (options.encoding === null) {
return this.parse(p, fixture)
}

switch (ext) {
case '.json': return this.parseJson(p, fixture)
case '.js': return this.parseJs(p, fixture)
case '.coffee': return this.parseCoffee(p, fixture)
case '.html': return this.parseHtml(p, fixture)
case '.png': case '.jpg': case '.jpeg': case '.gif': case '.tif': case '.tiff': case '.zip':
return this.parse(p, fixture, _.isNull(options.encoding) ? null : (options.encoding || 'base64'))
return this.parse(p, fixture, options.encoding)
default:
return this.parse(p, fixture, _.isNull(options.encoding) ? null : options.encoding)
return this.parse(p, fixture, options.encoding || 'utf8')
}
},

Expand Down Expand Up @@ -184,7 +192,7 @@ module.exports = {
.bind(this)
},

parse (p, fixture, encoding = 'utf8') {
parse (p, fixture, encoding) {
return fs.readFileAsync(p, encoding)
.bind(this)
},
Expand Down
16 changes: 16 additions & 0 deletions packages/server/test/unit/files_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,13 @@ describe('lib/files', () => {
})
})

// https://github.com/cypress-io/cypress/issues/1558
it('explicit null encoding is sent to driver as a Buffer', function () {
return files.readFile(this.projectRoot, 'tests/_fixtures/ascii.foo', { encoding: null }).then(({ contents }) => {
expect(contents).to.eql(Buffer.from('\n'))
})
})

it('parses json to valid JS object', function () {
return files.readFile(this.projectRoot, 'tests/_fixtures/users.json').then(({ contents }) => {
expect(contents).to.eql([
Expand Down Expand Up @@ -75,6 +82,15 @@ describe('lib/files', () => {
})
})

// https://github.com/cypress-io/cypress/issues/1558
it('explicit null encoding is written exactly as received', function () {
return files.writeFile(this.projectRoot, '.projects/write_file.txt', Buffer.from(''), { encoding: null }).then(() => {
return files.readFile(this.projectRoot, '.projects/write_file.txt', { encoding: null }).then(({ contents }) => {
expect(contents).to.eql(Buffer.from(''))
})
})
})

it('overwrites existing file by default', function () {
return files.writeFile(this.projectRoot, '.projects/write_file.txt', 'foo').then(() => {
return files.readFile(this.projectRoot, '.projects/write_file.txt').then(({ contents }) => {
Expand Down