-
Notifications
You must be signed in to change notification settings - Fork 55
fix: allow long remote URLs to be parsed #1381
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
base: main
Are you sure you want to change the base?
Conversation
| } | ||
| }), | ||
| ); | ||
| try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only added the try/catch block
| "p-queue": "^6.3.0", | ||
| "p-settle": "^3.1.0", | ||
| "p-timeout": "^3.2.0", | ||
| "parse-url": "^8.1.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eslint gets mad if we don't have this as a direct dependency but try to import it
| // Override the max input length to support long Bitbucket Server URLs | ||
| // The error is thrown in `parse-url` <- `git-up` <- `git-url-parse`, hence the odd modification here | ||
| const parseUrl = require('parse-url'); | ||
| parseUrl.MAX_INPUT_LENGTH = 3000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
per the recommendation in the error itself; require since it's pure JS
|
The issue is ready for review, and the acceptance criteria have been met:
|
1 similar comment
|
The issue is ready for review, and the acceptance criteria have been met:
|
|
|
||
| // Override the max input length to support long Bitbucket Server URLs | ||
| // The error is thrown in `parse-url` <- `git-up` <- `git-url-parse`, hence the odd modification here | ||
| const parseUrl = require('parse-url'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔎 Code Readability
Use ES6 import syntax to match the rest of the file: import * as parseUrl from 'parse-url';
Details
📖 Explanation: Mixing import styles reduces code consistency and the require() syntax doesn't provide TypeScript type checking benefits.
| const parseUrl = require('parse-url'); | |
| import * as parseUrl from 'parse-url'; |
e934614 to
93f7093
Compare
| this._onDidChangeBitbucketContext.fire(); | ||
| this._onDidChangeBitbucketContext.fire(); | ||
| } catch (err) { | ||
| err.subject_url = undefined; // remove potentially sensitive info |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔎 Code Design - Null Check
Accessing err.subject_url without checking if it exists could throw a runtime error if err is not an object or lacks this property.
What Is This Change?
Small fix to prevent an error in bitbucket logic if the remote URL is too long (the default value is 100 characters)
Source of the error:
parse-url<-git-up<-git-url-parseHow Has This Been Tested?
See loom:
https://www.loom.com/share/b59e9314d6694d37aaa491d6bb9db546
Basic checks:
npm run lintnpm run test