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: Custom dev ui port(load env from .env) #2702

Closed
wants to merge 19 commits into from

Conversation

FrankFang
Copy link

@FrankFang FrankFang commented Jun 6, 2023

OS: Windows 10
When I run yarn start, I get Something is already running on port 3000..
It's good if I can customize the dev port.

P.S. I am sure there is no server running on port 3000 because I just restarted my computer. Maybe something is broken but I no time to figure it out, so custimzation of port is needed for me.

scripts/start.mjs Outdated Show resolved Hide resolved
@FrankFang
Copy link
Author

I update a lot of code:

  1. Create neuron-shared as a shared package
  2. Add load-env.ts into neuron-shared and test it
  3. Upgrade dotenv from 8 to 16.1.4
  4. Use loadEnv to get env vars anywhere

@FrankFang FrankFang force-pushed the develop branch 2 times, most recently from 6f8dd90 to bdf06dd Compare June 6, 2023 17:43
package.json Outdated Show resolved Hide resolved
packages/neuron-shared/package.json Outdated Show resolved Hide resolved
packages/neuron-shared/package.json Outdated Show resolved Hide resolved
packages/neuron-shared/src/load-env.ts Show resolved Hide resolved
packages/neuron-shared/tests/load-env.test.ts Outdated Show resolved Hide resolved
packages/neuron-shared/tests/load-env.test.ts Show resolved Hide resolved
packages/neuron-shared/tests/load-env.test.ts Outdated Show resolved Hide resolved
packages/neuron-shared/tests/load-env.test.ts Outdated Show resolved Hide resolved
packages/neuron-wallet/package.json Outdated Show resolved Hide resolved
packages/neuron-wallet/src/env.ts Outdated Show resolved Hide resolved
@FrankFang
Copy link
Author

FrankFang commented Jun 6, 2023

It's late now, I'm going to sleep. I'll fix these later.

@FrankFang
Copy link
Author

Most are fixed but not 'node:fs’

packages/neuron-wallet/package.json Outdated Show resolved Hide resolved
packages/neuron-shared/tests/load-env.test.ts Outdated Show resolved Hide resolved
packages/neuron-shared/tests/load-env.test.ts Outdated Show resolved Hide resolved
packages/neuron-shared/tests/load-env.test.ts Outdated Show resolved Hide resolved
packages/neuron-ui/package.json Outdated Show resolved Hide resolved
@@ -17,8 +18,7 @@ export const loadEnv = () => {
for (let i = 0; i < dotenvFiles.length; i++) {
const dotenvFile = dotenvFiles[i]
if (fs.existsSync(dotenvFile)) {
dotenv.config({ path: dotenvFile })
break
dotenvExpand.expand(dotenv.config({ path: dotenvFile, override: false }))
Copy link
Author

@FrankFang FrankFang Jun 7, 2023

Choose a reason for hiding this comment

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

dotenvExpand makes ENV_2=${ENV_1} possible.

@yanguoyu
Copy link
Collaborator

yanguoyu commented Jun 8, 2023

With the PR, we can not auto-start ui with another port when run the command yarn start:ui if the default port has been used. And if the 3000 port has been used, we cannot get any help from the output. The neuron-wallet only waits for http://127.0.0.1:3000 because the neuron-ui does not throw any exceptions with the used port.

@FrankFang
Copy link
Author

The neuron-wallet only waits for http://127.0.0.1:3000

I've change the start task for neuron-wallet

"start": "node scripts/wait-ui.mjs && yarn run start:debug

So neuron-wallet could wait for http://127.0.0.1:${PORT} now.

@WhiteMinds
Copy link
Contributor

WhiteMinds commented Jun 12, 2023

My main concern currently is to avoid introducing the complexity of neuron-shared too early. At the moment, it only provides a unified rule for loading envFiles.
Which we can achieve using libraries like @next/env. This way, we can directly implement the same logic in both packages through simple code duplication, without the need for a new package implementation.

@Keith-CY @yanguoyu @devchenyan @JeffreyMa597 @homura , what do you think?

If we all agree to introduce neuron-shared, there are a few tasks that need to be done:

  1. Automatically compile on the first yarn, similar to this: https://github.com/nervosnetwork/neuron/blob/e7adec4b89ce9976ba9d598960a50f7dce176cff/package.json#LL38C38-L38C87
  2. Like neuron-wallet and neuron-ui, include Prettier and ESLint.
  3. Optionally, rename main.ts to a more conventional index.ts

@Keith-CY
Copy link
Collaborator

Keith-CY commented Jun 12, 2023

My main concern currently is to avoid introducing the complexity of neuron-shared too early. At the moment, it only provides a unified rule for loading envFiles. Which we can achieve using libraries like @next/env. This way, we can directly implement the same logic in both packages through simple code duplication, without the need for a new package implementation.

@Keith-CY @yanguoyu @devchenyan @JeffreyMa597 @homura , what do you think?

If we all agree to introduce neuron-shared, there are a few tasks that need to be done:

  1. Automatically compile on the first yarn, similar to this: e7adec4/package.json#LL38C38-L38C87
  2. Like neuron-wallet and neuron-ui, include Prettier and ESLint.
  3. Optionally, rename main.ts to a more conventional index.ts

It may be cumbersome to share an env through a package, it can be done simply by extending envs from the project root.

@WhiteMinds
Copy link
Contributor

It may be cumbersome to share an env through a package, it can be done simply by extending envs from the project root.

Currently, we are not sharing the environment variables themselves, but rather the loadEnv function. In reality, the environment configuration for each project is still independent. Therefore, I believe introducing this complexity (neuron-shared) is unnecessary.

@yanguoyu
Copy link
Collaborator

yanguoyu commented Jun 13, 2023

It may be cumbersome to share an env through a package, it can be done simply by extending envs from the project root.

Currently, we are not sharing the environment variables themselves, but rather the loadEnv function. In reality, the environment configuration for each project is still independent. Therefore, I believe introducing this complexity (neuron-shared) is unnecessary.

I strongly agree, and lerna can auto-load environment files when running tasks with lerna. https://lerna.js.org/docs/features/run-tasks#automatic-loading-of-env-files

@github-actions
Copy link
Contributor

Packaging for test is done in 5320392944 for commit 36c1621 .

@Keith-CY
Copy link
Collaborator

Will this PR be updated according to the comments above?

@Keith-CY Keith-CY closed this Jul 18, 2023
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.

5 participants