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

feat: add support for Node 17 #7710

Closed

Conversation

marvinroger
Copy link
Contributor

@marvinroger marvinroger commented Nov 18, 2021

New Pull Request Checklist

Issue Description

Related issue: #7708

Approach

Every test was failing, because (dns.lookup on Node.17 does not prioritize IPv4 addresses anymore), so reaching for example MongoDB on localhost resulted in a connect ECONNREFUSED ::1 error, because Mongo is not listening on the IPv6 interface.

I fixed it by manually overriding the resolution order in helper.js in tests.

TODOs before merging

  • Add changes to documentation (guides, repository pages, in-code descriptions)
  • A changelog entry is created automatically using the pull request title (do not manually add a changelog entry)

@parse-github-assistant
Copy link

parse-github-assistant bot commented Nov 18, 2021

Thanks for opening this pull request!

  • 🎉 We are excited about your hands-on contribution!

package.json Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Nov 19, 2021

Codecov Report

Merging #7710 (74d4ac1) into alpha (826aa79) will decrease coverage by 0.02%.
The diff coverage is 60.00%.

❗ Current head 74d4ac1 differs from pull request most recent head ffb7edc. Consider uploading reports for the commit ffb7edc to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##            alpha    #7710      +/-   ##
==========================================
- Coverage   93.98%   93.95%   -0.03%     
==========================================
  Files         183      183              
  Lines       13655    13655              
==========================================
- Hits        12833    12830       -3     
- Misses        822      825       +3     
Impacted Files Coverage Δ
src/Adapters/Auth/gcenter.js 98.24% <ø> (ø)
src/Options/Definitions.js 100.00% <ø> (ø)
src/Options/index.js 100.00% <ø> (ø)
src/batch.js 92.98% <ø> (-1.76%) ⬇️
src/cli/parse-server.js 32.81% <ø> (ø)
...dapters/Storage/Postgres/PostgresStorageAdapter.js 95.46% <45.45%> (ø)
.../Adapters/Storage/Postgres/PostgresConfigParser.js 100.00% <100.00%> (ø)
src/Controllers/LoggerController.js 91.57% <100.00%> (ø)
src/ParseServer.js 90.05% <100.00%> (ø)
src/middlewares.js 96.94% <100.00%> (ø)
... and 3 more

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 826aa79...ffb7edc. Read the comment docs.

@marvinroger marvinroger marked this pull request as ready for review November 19, 2021 08:07
@marvinroger
Copy link
Contributor Author

Should be good @mtrezza

README.md Outdated Show resolved Hide resolved
ci/ciCheck.js Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
spec/helper.js Outdated Show resolved Hide resolved
@mtrezza
Copy link
Member

mtrezza commented Dec 19, 2021

@marvinroger kindly let me know when this is ready for review

@marvinroger
Copy link
Contributor Author

@mtrezza This is ready for review, and all relevant tests pass.
I had to fix a missing await (probably because using 127.0.0.1 instead of localhost skips event-loop ticks because there's no need for DNS resolution?), and generate new X.509 test certificates with 127.0.0.1 as an alternative name (note it's still OK for localhost and, as the previous one, it is valid 100 years)

@mtrezza
Copy link
Member

mtrezza commented Dec 21, 2021

@davimacedo do you see any issue replacing localhost references with 127.0.0.1? See this comment why we do that for Node 17 compatibility and the related Node 17 PR.

If we make a change like this:

-options.serverURL = `http://localhost:${options.port}${options.mountPath}`;
+options.serverURL = `http://127.0.0.1:${options.port}${options.mountPath}`;
  • For logs and docs implies that Parse Server is only listening on 127.0.0.1, but in a dualstack (IPv4/6) environment it may well listen on 127.0.0.1 and :::1 which means localhost.
  • Dualstack is a solution for transitioning from IPv4 to v6, to keeping the agnostic localhost and add an option for either IPv4 or dualstack (whether to resolve IPv4 or IPv6 first) may be more future proof.
  • We would have to go though all the docs and replace localhost.
  • Possibly changes needed in the JS SDK as well, need to look into this.

@davimacedo
Copy link
Member

@mtrezza sorry for the delay. I'm ok in replacing localhost references with 127.0.0.1.

@mtrezza
Copy link
Member

mtrezza commented Jan 19, 2022

We may create an issue here if we require developers to not use localhost but 127.0.0.1 instead in the future, as it may be somewhat unintuitive and we could have a hard time reviewing this in every PR, unless we detect the term and add a CI check.

I understand from the previous comments that If we set dns.setDefaultResultOrder to resolve IPv4 first, it would still require some changes of localhost to 127.0.0.1, correct? Why was that?

@marvinroger
Copy link
Contributor Author

No, if you set dns.setDefaultResultOrder in your entrypoint, it's fine as is restores an iso behavior with Node 16. Which comments are you referring to?

@mtrezza
Copy link
Member

mtrezza commented Jan 19, 2022

I may have misread your previous comment, apologies. If setting dns.setDefaultResultOrder to IPv4 is all that's needed and we can keep the localhost references, maybe that's the way to go then?

In 217c526 this option is only added to the test helper, but localhost is also used in some files in src/.

I think with the change in helper the server runs fine in a test environment, but outside of tests where helper.js is not used, localhost still will default to IPv6. I'm unsure whether Parse Server needs a new option to set the DNS resolution order, which can then also be used in tests, also if we add IPv6 tests in the future. Or does Parse Server not need an option and that is something the developer has to take care of on a higher level, because it impacts the whole node.js process? But then how would that be done in a docker container or when launching Parse Server via CLI.

@mtrezza
Copy link
Member

mtrezza commented Mar 28, 2022

Closing in favor of #7896

Thanks for the investigation @marvinroger, after some more investigation I'll continue in #7896 with only setting the setDefaultResultOrder. That seems to be the most sustainable change.

@mtrezza mtrezza closed this Mar 28, 2022
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.

3 participants