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

Update blob-util to 2.0.2 #7795

Merged
merged 7 commits into from
Jul 7, 2020

Conversation

sainthkh
Copy link
Contributor

@sainthkh sainthkh commented Jun 24, 2020

User facing changelog

Breaking changes

  • The return type of the Cypress.Blob methods arrayBufferToBlob, base64StringToBlob, binaryStringToBlob, and dataURLToBlob have changed from Promise<Blob> to Blob.

Dependency Updates

  • Upgraded blob-utils from 1.3.3 to 2.0.2.

Additional details

How has the user experience changed?

Before

Cypress.Blob.base64StringToBlob(this.logo, 'image/png')
.then((blob) => {
  // work with the returned blob
})

After

const blob = Cypress.Blob.base64StringToBlob(this.logo, 'image/png')

// work with the returned blob

PR Tasks

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jun 24, 2020

Thanks for taking the time to open a PR!

@sainthkh sainthkh changed the base branch from develop to v5.0-release June 24, 2020 07:13
@jennifer-shehane jennifer-shehane added the type: breaking change Requires a new major release version label Jun 24, 2020
@jennifer-shehane jennifer-shehane removed the request for review from bahmutov June 24, 2020 08:11
@sainthkh
Copy link
Contributor Author

sainthkh commented Jun 24, 2020

firefox-5 is flaky failure. But firefox-8 is a bit weird. It failed exactly 63 tests but the test failed because of 62 !== 63.
When I tested it on local Ubuntu 20.04, it failed 75 tests.

@sainthkh sainthkh marked this pull request as ready for review June 24, 2020 08:36
@jennifer-shehane
Copy link
Member

@sainthkh Yeah, Chris has been working on fixing the firefox-8 flake, which has turned into more work than was anticipated. #7691

@chrisbreiding
Copy link
Contributor

I think this may need to be considered a breaking change, mostly because of the following in the changes:

Some APIs that returned Promises now return bare values.

We use the promise version of base64StringToBlob in this example in the docs. That doc will also need to be changed.

@flotwig flotwig mentioned this pull request Jun 26, 2020
21 tasks
@jennifer-shehane
Copy link
Member

@chrisbreiding this is a breaking change and pulled against the 5.0 release branch, so ok to merge in.

But yes, I would like a more descriptive explanation for what needs to change for user facing documentation, since I'm writing a migration guide. Also that example updated in a docs PR.

@sainthkh
Copy link
Contributor Author

@jennifer-shehane I guess the list of changed functions is good enough, right?

@jennifer-shehane
Copy link
Member

@sainthkh Sure, if you want to get something started or comment in here, I can work on more complete documentation of the change.

@sainthkh
Copy link
Contributor Author

@jennifer-shehane

The return type of the 4 APIs is changed from Promise<Blob> to Blob.

  • arrayBufferToBlob
  • base64StringToBlob
  • binaryStringToBlob
  • dataURLToBlob

2 more APIs are added:

  • arrayBufferToBinaryString
  • binaryStringToArrayBuffer

There're 2 ways to fix the example in the documentation.

Bare version:

// programmatically upload the logo
cy.fixture('images/logo.png').as('logo')
cy.get('input[type=file]').then(function($input) {
  // convert the logo base64 string to a blob
  const blob = Cypress.Blob.base64StringToBlob(this.logo, 'image/png')

  // pass the blob to the fileupload jQuery plugin
  // used in your application's code
  // which initiates a programmatic upload
  $input.fileupload('add', { files: blob })
})

Promise-wrapped version:

// programmatically upload the logo
cy.fixture('images/logo.png').as('logo')
cy.get('input[type=file]').then(function($input) {
  // convert the logo base64 string to a blob
  new Promise((resolve) => {
    const blob = Cypress.Blob.base64StringToBlob(this.logo, 'image/png')

    resolve(blob)
  }).then((blob) => {
    // pass the blob to the fileupload jQuery plugin
    // used in your application's code
    // which initiates a programmatic upload
    $input.fileupload('add', { files: blob })
  })
})

I personally prefer the first version because it's simpler.

We don't have to change the second example, because imgSrcToDataURL is not changed.

@jennifer-shehane
Copy link
Member

Opened a PR for docs here: cypress-io/cypress-documentation#2938

@brian-mann
Copy link
Member

If we really want to be nice to our users... we should override the .then() function on the old promise returning API's and tell them what to change and why this is no longer the case.

I imagine not many users actually use these functions (unless it is used internally in a 3rd party plugin somewhere), so that may be overkill.

@jennifer-shehane
Copy link
Member

@brian-mann Everyone that tests a file upload uses this method, so I would expect that to be a lot of people actually - and some of the plugins made for file upload.

@sainthkh
Copy link
Contributor Author

To implement Brian's idea, we need to add then() function to Blob object. It's a bit weird but I don't it's impossible. Should I go on?

@brian-mann
Copy link
Member

@sainthkh if you have the time then I don't see why not.

@sainthkh
Copy link
Contributor Author

sainthkh commented Jul 1, 2020

@brian-mann I tried it and it isn't simple. Because webpack makes the members of Cypress.Blob immutable.

@sainthkh
Copy link
Contributor Author

sainthkh commented Jul 2, 2020

I found the way and Cypress.Blob now shows error messages when a user tries to use then() method.

Test code:

it('binaryStringToBlob', () => {
  Cypress.Blob.binaryStringToBlob('0100101').then((blob) => {
    // it should fail.
  })
})

Result Image:

Screenshot from 2020-07-02 18-14-05

Copy link
Member

@jennifer-shehane jennifer-shehane left a comment

Choose a reason for hiding this comment

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

Nice! Just some suggestions on the wording of this message.

packages/driver/src/cypress/error_messages.js Outdated Show resolved Hide resolved
@jennifer-shehane jennifer-shehane self-requested a review July 6, 2020 09:08
@jennifer-shehane jennifer-shehane merged commit 8971ad1 into cypress-io:v5.0-release Jul 7, 2020
@brian-mann
Copy link
Member

The tests in this PR are incorrect. The cy.on('fail') tests must use a done() callback otherwise there is no guarantee that the callback is ever called.

Comment on lines +2 to +10
it('arrayBufferToBlob', () => {
cy.on('fail', (err) => {
expect(err.message).to.include('no longer returns a `Promise`')
})

Cypress.Blob.arrayBufferToBlob('1234').then((blob) => {
// it should fail.
})
})
Copy link
Member

Choose a reason for hiding this comment

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

A (done) callback parameter must be yielded to guarantee that the cy.on('fail', ...) hits accordingly otherwise the test can pass if the error handle is not invoked.

it('arrayBufferToBlob', (done) => {
    cy.on('fail', (err) => {
      expect(err.message).to.include('no longer returns a `Promise`')
	  done()
    })

    Cypress.Blob.arrayBufferToBlob('1234').then((blob) => {
      // it should fail.
    })
  })

@brian-mann
Copy link
Member

I will update the tests in this branch directly, but noting the changes here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: breaking change Requires a new major release version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace dep @types/blob-util (it's deprecated)
4 participants