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

Change node default port #739

Merged
merged 7 commits into from
Jan 11, 2022
Merged

Change node default port #739

merged 7 commits into from
Jan 11, 2022

Conversation

fishmonger45
Copy link
Contributor

  • Changes the default port of the node service to 3001 and keeps the default port of the query service as 3000
  • Reason for decision above is that people will be used to going to localhost:3000 in their browsers so we don't want to change this
  • Adds a log for the URL of the playground as a shortcut
  • Add strict options to getYargsOptions so that invalid flags are not swallowed
  • I need to change the docker images on the templates so that they support the new ports so let me put that up before this is merged

@ianhe8x
Copy link
Collaborator

ianhe8x commented Jan 9, 2022

@wqsz7xn I didn't notice that you have added strictOptions to subql-node. I have a concern that this might break something from hosted service, because with this option, they have to handle the args in different subql version in a precious manner now.

@zhex

@fishmonger45
Copy link
Contributor Author

fishmonger45 commented Jan 9, 2022

@ianhe8x @zhex
I thought strictOptions would be good to have because sometimes when typing out a longish command you make a mistake and it gets swallowed and starts (with the bad configuration). But I didn't know that hosted team would have problems. Let me know if you want to revert the strict options stuff, we can maybe workaround with a warning or just drop this all together

@zhex
Copy link
Contributor

zhex commented Jan 10, 2022

I think both changes will affect hosted service. all the existing deployments in the hosted service are running on the default port, which means we need to force set a port after this version is released. The hosted service need to support different versions in one template, restrictOptions might cause some legacy options stop working.

@fishmonger45
Copy link
Contributor Author

strictOptions has been removed and the default port of 3000 added re-added a fallback for query. Is there anything that I can add on my side to help the hosted team transition the new node port?

@ianhe8x
Copy link
Collaborator

ianhe8x commented Jan 10, 2022

@wqsz7xn how about instead of change the port to 3001, we leave it 3000 unchanged but detect when there's port conflict, we increase 1 until finding an available port.

@fishmonger45
Copy link
Contributor Author

fishmonger45 commented Jan 10, 2022

how about instead of change the port to 3001, we leave it 3000 unchanged but detect when there's port conflict, we increase 1 until finding an available port.

Now that I think about it this is a bit weird and would mean that ports are different across environments, also weird that we are port scanning on one service but not the other (we could do both). How about we just use some other unbound port? We have a envvar in place if people want to change the port themselves. I might be missing some advantage to port scanning though

@ianhe8x
Copy link
Collaborator

ianhe8x commented Jan 10, 2022

create-react-app use this approach. https://github.com/facebook/create-react-app/blob/f0a837c1f07ebd963ddbba2c2937d04fc1b79d40/packages/react-dev-utils/WebpackDevServerUtils.js

This will only be used when no port is specified via env or args.

@fishmonger45
Copy link
Contributor Author

fishmonger45 commented Jan 10, 2022

We have decided against doing it the createreactapp way because we don't really want user input here and will do a few tries against an iterated port. They can set their own port if they really want it

}
})
.catch((err) => {
console.error(err);
Copy link
Collaborator

Choose a reason for hiding this comment

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

clean up console.error, or print only when --debug is used.

packages/node/src/main.ts Outdated Show resolved Hide resolved
packages/query/src/main.ts Outdated Show resolved Hide resolved
@@ -84,6 +86,10 @@ export class GraphqlModule implements OnModuleInit, OnModuleDestroy {
cors: true,
});

if (this.config.get('playground')) {
logger.info('Started playground at http://localhost:3000');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Print the actual used port.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh yeah


export async function findAvailablePort(startPort: number, range = 10): Promise<number> {
for (let port = startPort; port <= startPort + range; port++) {
const _port = await detectPort(port).catch(() => {
Copy link
Collaborator

@ianhe8x ianhe8x Jan 11, 2022

Choose a reason for hiding this comment

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

Suggested change
const _port = await detectPort(port).catch(() => {
try{
const _port = await detectPort(port)
...
} catch(e) {
return null;
}

just code styling

@ianhe8x ianhe8x merged commit d9f76d2 into subquery:main Jan 11, 2022
bz888 pushed a commit that referenced this pull request Jun 3, 2022
* change node default port

* remove strictOptions from node and query and add fallback port to query

* add port scan

* more tests and change promise

* add sanitization to arguments

* code style
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