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 use to be compatible with Cypress 5.0 #215

Merged
merged 6 commits into from
Aug 27, 2020

Conversation

emilong
Copy link
Contributor

@emilong emilong commented Aug 19, 2020

Wraps return value of Cypress.Blob.base64StringToBlob() in a Cypress.Promise.resolve, which will allow us to treat it as a Promise no matter what. If it's already a Promise, as it was in Cypress <=4, this will be also be compatible.

Fixes #214

slt
slt previously approved these changes Aug 20, 2020
EtienneBruines
EtienneBruines previously approved these changes Aug 20, 2020
@mduft
Copy link

mduft commented Aug 20, 2020

Any way I can test this? I tried:

  • Clone & checkout of this branch
  • npm run-script build
  • npm pack
  • On my project: npm install ../xxxx/cypress-file-upload-4.0.7.tgz
  • Verified that the bundle.js contains the updated code
  • Run cypress -> I still get the same error - why?

@EtienneBruines
Copy link

Cypress adds a .then function to the result of e.g. base64StringToBlob

Soure: https://github.com/cypress-io/cypress/blob/de175d910d02c8d082b05883885c8a7aa1fb27b7/packages/driver/src/util/breaking_change_warning.ts#L18

        val.then = function () {
          $errUtil.throwErrByPath('breaking_change.blob_util2', {
            args: {
              functionName: key,
            },
          })
        }

Perhaps wrapping it around Cypress.Promise.resolve(...) still invokes the .then method that was added by Cypress?

According to the bluebird docs:

http://bluebirdjs.com/docs/api/promise.resolve.html
If value is a thenable (Promise-like object, like those returned by jQuery's $.ajax), returns a trusted Promise that assimilates the state of the thenable.

@mduft
Copy link

mduft commented Aug 20, 2020

Maybe @jennifer-shehane can shed some light on this? What else would have to be done to have a working blob-util again?

@chrisbreiding
Copy link
Contributor

We added the .then to try and add a better error message when using the old API, but now it's clear that wasn't a great idea since it gets confused for a 'thenable'.

You should be able to work around it by using Promise.try instead of Promise.resolve:

// before:
Cypress.Promise.resolve(Cypress.Blob.base64StringToBlob(fileContent, mimeType))
// after:
Cypress.Promise.try(() => Cypress.Blob.base64StringToBlob(fileContent, mimeType)))

@mduft
Copy link

mduft commented Aug 20, 2020

Cypress.Promise.try(() => Cypress.Blob.base64StringToBlob(fileContent, mimeType)))

I tried with this code instead, but I get the very same result...

base64StringToBlob() no longer returns a Promise. Update the use of base64StringToBlob() to expect a returned Blob.

@emilong emilong dismissed stale reviews from EtienneBruines and slt via 57def11 August 20, 2020 13:51
Co-authored-by: Chris Breiding <chrisbreiding@users.noreply.github.com>
@emilong
Copy link
Contributor Author

emilong commented Aug 20, 2020

Thanks, @chrisbreiding! I missed that detail... still a bit early in the morning 😅 ☕

@EtienneBruines
Copy link

Cypress.Promise.try does not mean "meh, I don't care if it works" though.

It's just a fancy way of handling the non-async part of the potential errors.

http://bluebirdjs.com/docs/api/promise.try.html
Start the chain of promises with Promise.try. Any synchronous exceptions will be turned into rejections on the returned promise.

@chrisbreiding
Copy link
Contributor

That's my bad. I suggested it before trying it out. I had hoped it wouldn't trigger using .then on the return value like Promise.resolve, but it looks like it does.

Might have to resort to an ugly hack to fix this. Something like the following:

const removeThen = (value) => {
  if (value) value.then = undefined

  return value
}

Cypress.Promise.resolve(removeThen(Cypress.Blob.base64StringToBlob(fileContent, mimeType)))

@emilong
Copy link
Contributor Author

emilong commented Aug 20, 2020

I'm thinking we'll want to condition that on the Cypress version too? To be compatible with < 5.0.0. I'll draft it up.

@chrisbreiding
Copy link
Contributor

Sorry for the hassle with this one. We'll improve this for the next release, but unfortunately the hack will have to remain for users on 5.0.0.

@emilong
Copy link
Contributor Author

emilong commented Aug 20, 2020

Ok, I actually did due diligence this time and confirmed efa9358 works on my tests locally for 4.12.1 and 5.0.0. 🤞

@mduft
Copy link

mduft commented Aug 21, 2020

Yeah, I can confirm that this fix work perfectly fine now :)

mduft
mduft previously approved these changes Aug 21, 2020
@KatieMFritz
Copy link

Is there an estimate for when this might be merged for the plugin? I'm trying to use the PR branch but I'm running into some issues (webpack can't resolve the module).

@mduft
Copy link

mduft commented Aug 24, 2020

MaintainerNotAvailableException?

gregberge
gregberge previously approved these changes Aug 24, 2020
@dragonfire1119
Copy link

Hope this gets merged soon this works perfectly!

@KatieMFritz
Copy link

KatieMFritz commented Aug 24, 2020

Can anyone help me figure out how to use the https://github.com/binti-family/cypress-file-upload/tree/cypress-v5-compat branch in my build?

What I did:

  1. Delete cypress-file-upload from node_modules directory.
  2. Delete yarn.lock.
  3. Add to devDependencies in package.json: "cypress-file-upload": "binti-family/cypress-file-upload#cypress-v5-compat",
  4. yarn install
  5. Add to cypress/support/commands.js: import 'cypress-file-upload'

When I run my test, I get this error:

rror: Webpack Compilation Error
./cypress/support/commands.js
Module not found: Error: Can't resolve 'cypress-file-upload' in '/[myroot]/cypress/support'
resolve 'cypress-file-upload' in '[myroot]/cypress/support'
  Parsed request is a module
  using description file: [myroot]/package.json (relative path: ./cypress/support)
    Field 'browser' doesn't contain a valid alias configuration
    Looked for and couldn't find the file at the following paths:

When I inspect the import command in my IDE, I see it's linking to node_modules/cypress-file-upload/types/index.d.ts.

I'm using yarn workspaces, so my node_modules directory is a higher up than my package.json where I'm requiring stuff.

===
SOLVED by dragonfire1119!

cd /node_modules/cypress-file-input
yarn
yarn build

@emilong emilong dismissed stale reviews from gregberge and mduft via 88bc2ae August 26, 2020 20:17
@emilong
Copy link
Contributor Author

emilong commented Aug 26, 2020

Ok! Updated to use semver for comparison and I tried it locally on Cypress 4 and 5. Would appreciate anyone else double checking though!

abramenal
abramenal previously approved these changes Aug 26, 2020
@josephzidell
Copy link
Contributor

@abramenal Before this gets merged, I asked the author to try a different way that introduces no new dependencies and might also be safer. Hopefully we can wait 1 more day for that.

@emilong
Copy link
Contributor Author

emilong commented Aug 26, 2020

I had a chance to update to use @josephzidell's approach and check it on Cypress 4 and 5 again.

@josephzidell josephzidell merged commit 89efcd7 into abramenal:master Aug 27, 2020
@josephzidell
Copy link
Contributor

LGTM

@josephzidell
Copy link
Contributor

@all-contributors add @emilong for code

@allcontributors
Copy link
Contributor

@josephzidell

I've put up a pull request to add @emilong! 🎉

@jojost1
Copy link

jojost1 commented Aug 27, 2020

Will you also make a new release?

@josephzidell
Copy link
Contributor

We're working on it 🍻

@abramenal
Copy link
Owner

@all-contributors add @josephzidell for maintenance

@allcontributors
Copy link
Contributor

@abramenal

I've put up a pull request to add @josephzidell! 🎉

@abramenal
Copy link
Owner

Thanks everyone for working on this, v4.1.0 is out 🎉🎉🎉

@josephzidell
Copy link
Contributor

Unfortunately, Cypress 5 is still showing this error:

binaryStringToBlob() no longer returns a Promise. Update the use of binaryStringToBlob() to expect a returned Blob

@testtek
Copy link

testtek commented Aug 27, 2020

Thank you !!! I can confirmed that it worked !! 👍

@martinsoenen
Copy link

It worked for me too !! Thanks a lot

@bbonevPIT
Copy link

I just pulled the latest code, I can confirm the file upload is working on the latest cypress version, thank you so much guys!

@emilong emilong deleted the cypress-v5-compat branch August 27, 2020 16:57
@JohnnyChiang
Copy link

Unfortunately, Cypress 5 is still showing this error:

binaryStringToBlob() no longer returns a Promise. Update the use of binaryStringToBlob() to expect a returned Blob

Having the same error here

@Ksan8
Copy link

Ksan8 commented Aug 28, 2020

I'm also still getting the error that @josephzidell and @JohnnyChiang mentioned. Going to downgrade again until this is stable.

@emilong
Copy link
Contributor Author

emilong commented Aug 28, 2020

@Ksan8 @josephzidell @JohnnyChiang can you say more about your set up? E.g. I'm using cypress to drive chrome only and I didn't have any other plugins/extensions that hit this issue. Those are the first two things that come to mind as being possibilities?

@saschwarz
Copy link

@Ksan8 @josephzidell @JohnnyChiang take a look at #222 and #225 to see if that is your use case. Also, 4.1.1 has been released with a fix.

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

Successfully merging this pull request may close these issues.

File upload failures with new Cypress v5.0.0