refactor(common): nuxt.config.js and dynamic server options (latest process.env)

[galvez]

Shouldn't be needed after #4207 (comment)

[galvez]

@pi0 or maybe not? will check later, gtg now :)

[sibbng]

I think they are needed for start your application from command line.

[aldarund]

Tests failing so not so easy :) But not a good that it repeats

[pi0]

@pimlie is our cli hero :D

[pimlie]

It seems to me this could be merged after changing the test. The failing test is a direct result of the expectation that ./utils.js set a default for host/port. If we want to improve that by moving it to nuxt.config.js like all other defaults then we just cant test for that here.

-- edit --
And isnt the whole getLatestHost function redundant? Except for argv. everything is already set in nuxt.config.js. I think we could remove that and change it just to

if (argv.port) {
  options.server.port = argv.port
}

Then if no port is supplied on the commandline the same defaults as now are already set in nuxt.config.js and applied to the running config with defaultsDeep.

[galvez]

@pimlie ha, you're totally right!

I had a feeling about this too but couldn't look into it at the time. Will update.

[galvez]

So I've stopped at trying to load a fresh version of NuxtConfig because we need to pick up fresh process.env values. Suggestions welcome :)

[galvez]

Back later!

[pimlie]

Why and where is the fresh config needed? Its a bit hard to follow your thought process with all the fix descriptions, but if you are trying to fix the failing test which checks env-host then you will never succeed in that ;)

These are all strict unit tests which (try) to only test one (class) method at the time (and only stuff from this package). As setting the host from env is now moved from this package to nuxt/common the env-host test can be removed here I guess (and it should be added to a nuxt/common test).

[galvez]

Why and where is the fresh config needed?

Because we introduce new environment variables dynamically in a test. But I guess you're right, following this approach this test doesn't make sense anymore, as there's no real-world scenario where process.env would be actually modified right before loadNuxtConfig().

[pimlie]

Ah ok, thanks. If we have that problem in nuxt/common then its probably easier to just call jest.resetModules() or (sometimes that doesnt work) delete require.cache[...path.to.nuxt-config] as it seems it'd only be a test issue and not a real world issue.

[codecov-io]

Codecov Report

Merging #4208 into dev will decrease coverage by 0.16%.
The diff coverage is 16.66%.

@@           Coverage Diff            @@
##            dev    #4208      +/-   ##
========================================
- Coverage    89%   88.83%   -0.17%     
========================================
  Files        39       42       +3     
  Lines      1691     1684       -7     
  Branches    443      436       -7     
========================================
- Hits       1505     1496       -9     
- Misses      159      161       +2     
  Partials     27       27

Impacted Files	Coverage Δ
packages/common/src/config/render.js	0% <0%> (ø)
packages/common/src/config/build.js	0% <0%> (ø)
packages/common/src/config/server.js	100% <100%> (ø)
packages/common/src/config/index.js	14.28% <14.28%> (ø)
packages/cli/src/utils.js	83.72% <33.33%> (-6.48%)	⬇️

---

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 00e83e3...dca26a6. Read the comment docs.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.