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.

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

fix(storage-client): parseFromString requires valid mimeType #424

Closed
wants to merge 1 commit into from
Closed

fix(storage-client): parseFromString requires valid mimeType #424

wants to merge 1 commit into from

Conversation

krystofwoldrich
Copy link

This PR fixes error caused by recent version of https://www.npmjs.com/package/@xmldom/xmldom/v/0.9.0 where mimeType became required argument.

# This file contains the result of Yarn building a package (appium-chromedriver@npm:5.6.73)
# Script name: postinstall

ERR! CDInstaller Error installing Chromedriver: DOMParser.parseFromString: the provided mimeType "undefined" is not valid.
ERR! CDInstaller TypeError: DOMParser.parseFromString: the provided mimeType "undefined" is not valid.
ERR! CDInstaller     at DOMParser.parseFromString (/Users/krystofwoldrich/repos/sentry-react-native/dev-packages/e2e-tests/node_modules/@xmldom/xmldom/lib/dom-parser.js:215:9)
ERR! CDInstaller     at parseGoogleapiStorageXml (/Users/krystofwoldrich/repos/sentry-react-native/dev-packages/e2e-tests/node_modules/appium-chromedriver/lib/storage-client/googleapis.js:98:31)
ERR! CDInstaller     at ChromedriverStorageClient.retrieveMapping (/Users/krystofwoldrich/repos/sentry-react-native/dev-packages/e2e-tests/node_modules/appium-chromedriver/lib/storage-client/storage-client.js:97:59)
ERR! CDInstaller     at ChromedriverStorageClient.syncDrivers (/Users/krystofwoldrich/repos/sentry-react-native/dev-packages/e2e-tests/node_modules/appium-chromedriver/lib/storage-client/storage-client.js:357:7)
ERR! CDInstaller     at install (/Users/krystofwoldrich/repos/sentry-react-native/dev-packages/e2e-tests/node_modules/appium-chromedriver/lib/install.js:57:3)
ERR! CDInstaller     at Object.doInstall (/Users/krystofwoldrich/repos/sentry-react-native/dev-packages/e2e-tests/node_modules/appium-chromedriver/lib/install.js:64:3)
ERR! CDInstaller     at main (/Users/krystofwoldrich/repos/sentry-react-native/dev-packages/e2e-tests/node_modules/appium-chromedriver/install-npm.js:66:5)
ERR! CDInstaller Downloading Chromedriver can be skipped by setting the'APPIUM_SKIP_CHROMEDRIVER_INSTALL' environment variable.

@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Aug 30, 2024 •
edited
Loading

CLA Signed

  • ✅login: krystofwoldrich / (ad7a45a)

The committers listed above are authorized under a signed CLA.

KazuCocoa
KazuCocoa reviewed Aug 30, 2024
View reviewed changes
lib/storage-client/googleapis.js
@@ -95,7 +95,7 @@ export function parseNotes(content) {
* @returns {Promise<ChromedriverDetailsMapping>}
*/
export async function parseGoogleapiStorageXml(xml, shouldParseNotes = true) {
const doc = new DOMParser().parseFromString(xml);
const doc = new DOMParser().parseFromString(xml, 'application/xml');
Copy link
Member

Choose a reason for hiding this comment

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

Could you import MIME_TYPE like appium/appium#20513 instead of giving vanilla value?
It is appreciated to set ^0.9 for https://github.com/appium/appium-chromedriver/blob/master/package.json#L47 as well

Copy link
Contributor

@mykola-mokhnach mykola-mokhnach Aug 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

yes, I believe we need to import the same constant as in https://github.com/appium/appium/pull/20513/files

Copy link
Contributor

Choose a reason for hiding this comment

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

also it definitely makes sense to bump the minimum version of xmldom in package.json to 0.9.0

@muvaf
Copy link

muvaf commented Aug 30, 2024

Thanks for the change! I'm using CHROMEDRIVER_VERSION=113.0.5672.63 and installing appium-chromedriver@5.6.69, both with hard-coded version but the daily image build still started to fail with this error. Is it because versions in package.json are not static?

@krystofwoldrich
Copy link
Author

Yes, exactly,
existing install using 0.8.10 or older will work. But new installs will resolve to 0.9.0 and fail.

@krystofwoldrich
Copy link
Author

https://www.npmjs.com/package/@xmldom/xmldom/v/0.9.0 was released ~20 hours ago so the image should start failing in one of the future runs.

@muvaf
Copy link

muvaf commented Aug 30, 2024

@krystofwoldrich Would it be possible to change the version from 0.x to 0.9.0 to prevent it? I see source-map-support also uses 0.x which may cause the same issue at one point though I'm sure there were historical reasons these are set to 0.x

@mykola-mokhnach mykola-mokhnach mentioned this pull request Aug 30, 2024
Merged
@mykola-mokhnach
Copy link
Contributor

Closed in favour of #425

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mykola-mokhnach mykola-mokhnach mykola-mokhnach left review comments

@KazuCocoa KazuCocoa KazuCocoa 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.
4 participants
@krystofwoldrich @muvaf @mykola-mokhnach @KazuCocoa