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

fix: escape IPv6 address in Node 17 #2334

Closed
wants to merge 3 commits into from
Closed

Conversation

eight04
Copy link
Contributor

@eight04 eight04 commented Oct 25, 2021

Part of #2331.

@codecov
Copy link

codecov bot commented Oct 25, 2021

Codecov Report

Merging #2334 (d0c20e7) into master (96d40a1) will decrease coverage by 0.05%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2334      +/-   ##
==========================================
- Coverage   99.88%   99.82%   -0.06%     
==========================================
  Files          32       32              
  Lines        1699     1702       +3     
==========================================
+ Hits         1697     1699       +2     
- Misses          2        3       +1     
Impacted Files Coverage Δ
src/extension-runners/chromium.js 99.23% <66.66%> (-0.77%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96d40a1...d0c20e7. Read the comment docs.

@eight04
Copy link
Contributor Author

eight04 commented Oct 25, 2021

Hmm perhaps we should just change localhost to 127.0.0.1 and close this PR?

{port: 0, host: 'localhost'},

Copy link
Member

@rpl rpl left a comment

Choose a reason for hiding this comment

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

@eight04 thanks for filing and investigating #2331 ❤️

If I'm reading #2331 correctly, to fully fix the web-ext run -t chromium on nodejs 17 does require that the chrome-launcher issue has to be fixed as well, is that right?

If you have already filed a github issue in the chrome-launcher repo, would you mind to mention it in the issue and pull request description? (just so that we can track it the upstream issue and adjust our plans with the web-ext side of the issue accordingly).

Follows an initial round of review comments.

Would you mind to also remove the package-lock.json changes? (to initialize the repo without making npm to regenerate this lock file into its v2 format you can use npm ci instead of npm install).

@@ -424,3 +424,10 @@ export class ChromiumExtensionRunner {
}
}
}

export function escapeIPv6(address: string): string {
if (/^([a-f0-9:]+:+)+[a-f0-9]+$/.test(address)) {
Copy link
Member

Choose a reason for hiding this comment

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

it looks that nodejs does provide a net.isIPv6 method since nodejs 0.3.0:

Personally I think I would prefer using that nodejs API method here to detect if the string is an IPv6 address instead of a regular expression.

Given that this method is also exported, we may cover it with a simple unit test (and that should also make codecov happy).

Copy link
Member

Choose a reason for hiding this comment

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

Another testing-related detail: along with covering the escapeIPV6 method with a unit test, we should also double-check how we may also want to assert in tests defined in test.chromium.js test file that escapeIPV6 has been called internally by the ChromiumExtensionRunner class, and so we would need it to be wrapper or replaced in the test with a sinon spy.

To make our life easier, we may actually see if we can just add it as an optional parameter of the createReloadManagerExtension method, set to the esacpeIPV6 function here by default, then in the test we may call this method directly along with passing to it the sinon.spy, and finally assert that we got the expected result and that the sinon.spy has been called the expected number of times and with the expected parameters.

@@ -126,7 +127,7 @@ describe('util/extension-runners/chromium', async () => {

// $FlowIgnore: allow to call addess even wss property can be undefined.
const wssInfo = runnerInstance.wss.address();
const wsURL = `ws://${wssInfo.address}:${wssInfo.port}`;
const wsURL = `ws://${escapeIPv6(wssInfo.address)}:${wssInfo.port}`;
Copy link
Member

Choose a reason for hiding this comment

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

Nit, to avoid having to recall to do this escaping manually, it seems to me that it may be reasonable to create a method that return a ws url given address and port, how that sounds?

@@ -17,7 +17,7 @@ exports.flowCheck = () => {
};

exports.flowStatus = () => {
const res = spawnSync('flow', ['status'], {stdio: 'inherit'});
const res = spawnSync('flow', ['status'], {stdio: 'inherit', shell: true});
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing that adding shell: true here is making this script able to run flow on Windows 10 (and it was not working without it), is that right?

Would you mind to split this out in a separate PR as a single commit with the following commit message (following the conventional changelog format)?
`"chore: Fixed issue with running flow checks on Windows"

we could merge that part right away.

@rpl rpl mentioned this pull request Oct 26, 2021
5 tasks
@rpl
Copy link
Member

rpl commented Oct 26, 2021

Hmm perhaps we should just change localhost to 127.0.0.1 and close this PR?

{port: 0, host: 'localhost'},

@eight04 eh, I completely missed to notice this comment, and I just jumped to look to the PR first.

That seems to be another option that we can consider.

Given that we still need the dependencies that are breaking due to this nodejs 17 breaking change, it looks that we may need to track some issues filed upstream and some in web-ext itself to make web-ext to fully support nodejs 17, I filed #2337 as a meta bug to tracking the issues related to nodejs 17 compatibility issues.

Out of curiosity: have you tried if nodejs --dns-result-order ipv4first path/to/web-ext ... works as a workaround in the meantime? (it is briefly mentioned in this comment nodejs/node#40537 (comment))

@eight04
Copy link
Contributor Author

eight04 commented Oct 27, 2021

Out of curiosity: have you tried if nodejs --dns-result-order ipv4first path/to/web-ext ... works as a workaround in the meantime? (it is briefly mentioned in this comment nodejs/node#40537 (comment))

Yes this works. Chrome can be connected correctly and I don't need to specify --adb-host for firefox-android.

I'll close this and file two new PRs for flow-check on Windows and to replace localhost with ipv4 which should be simpler.

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.

2 participants