fix: usage of providerUrl in fuels dev command
#3528
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
nedsalk
requested review from
digorithm,
arboleya,
Torres-ssf,
Dhaiwat10,
danielbate,
petertonysmith94 and
maschad
as code owners
nedsalk
force-pushed
the
ns/fix/fuels-dev-ephemeral-port
branch
from
d43f113 to
84095cd
Compare
| @@ -124,7 +124,8 @@ | |||
| "vite-plugin-json5": "1.1.2", | |||
| "vite-plugin-node-polyfills": "0.22.0", | |||
| "vite-plugin-plain-text": "1.4.2", | |||
| "vitest": "2.0.5" | |||
| "vitest": "2.0.5", | |||
| "fuels": "workspace:*" | |||
There was a problem hiding this comment.
I had to add this so that I could run this command in the test. This is because the spawn is running in the root of the repo and there is no fuels dependency there that it can run. Upon adding it, the spawn runs as expected.
| const providerUrl = `http://${accessIp}:${port}/v1/graphql`; | ||
|
|
||
| const { cleanup, snapshotDir } = await launchNode({ | ||
| const { cleanup, url, snapshotDir } = await launchNode({ |
There was a problem hiding this comment.
I think this PR could be reduced to this:
const port = config.fuelCorePort ?? 0;
const { cleanup, snapshotDir, url: providerUrl } = await launchNode({And then changing the title to something like:
fix: usage of `providerUrl` in `fuels dev` commandThen, remove the portfinder if it's not used elsewhere.
There was a problem hiding this comment.
I did consider this but was hesitant to suggest because it would be a breaking change due to the default previously being port 4000 and now it'd become a random port unless specified by the user.
I don't see a way to mitigate this breaking change technically. For example, if we set a default value for config.fuelCorePort to 4000 and then, if launchNode fails to create a node on that port that we retry with port 0, we wouldn't be able to differentiate between the default 4000 and an explicit 4000 that the user set and for which we don't want to default to 0 and retry.
We could, however, just make the PR a breaking change one and tell the users that they have to specify the fuelCorePort value now if they want their fuel-core node to always run on the same port.
There was a problem hiding this comment.
How would it be a breaking change if the port is not static? Besides, if we can't start the node on the port users chose (if they do choose a port), we just throw and exit. Retries make no sense here.
There was a problem hiding this comment.
It would always start at 4000 and probably would always stay at 4000 for the majority of users and people might have taken it as a static default, hence why I saw it as a breaking change. But you're right, we didn't guarantee it being static. I made the changes, please review again.
There was a problem hiding this comment.
The tests are now failing, but you have a good point: we must consider these soft changes on behalf of users since 4000 is usually the default port used to start fuel-core, and users may be accustomed to it.
If we default to 0, which port does the search start at?
There was a problem hiding this comment.
It will be a random port assigned by the OS in the ephemeral port range. The below command gives the range in linux systems and is 32768 to 60999.
sysctl net.ipv4.ip_local_port_range
# net.ipv4.ip_local_port_range = 32768 60999There was a problem hiding this comment.
Ok, it's way distant than the default 4000.
Let me go back for a second here: Why can't we continue to use getPortPromise()?
const port = config.fuelCorePort ?? (await getPortPromise({ port: 4000 }));
const { cleanup, snapshotDir, url: providerUrl } = await launchNode({
nedsalk
changed the title
0 port for the fuelCorePort fieldproviderUrl in fuels dev command
|
Coverage Report:
Changed Files:
|
fuelCorePort: 0infuels.config.ts#3527Summary
The
autoStartFuelCorefunction wasn't using the provider url returned bylaunchNodebut was rather constructing its own url, which led to the inability to use the ephemeral port0because it'd construct an invalidhttp://127.0.0.1:0/v1/graphqlurl.Checklist