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(e2e): support env option for startServer #640

Merged
merged 12 commits into from
Jan 17, 2024

Conversation

BobbieGoede
Copy link
Member

Not sure where this function should be added so I made a module-utils directory and added it as an export, I had to add failOnWarn: false too as it was exiting due to a warning: Inlined implicit external scule.

This PR adds a module testing utility that I made for testing runtime config changes in @nuxtjs/i18n. We are using a copy of the old @nuxt/test-utils inside our repo and I have made some changes that suit our needs, we hope to use this repo in the future again.

This utility function has been used in nuxt-modules/i18n#2576, it accepts an object matching the runtimeConfig property in nuxt.config.ts, which gets flattened, formatted and passed to the startServer env variable, so the server restarts with the new environment variables.

example:

const restoreConfig = await setRuntimeConfig({
  public: {
    i18n: {
      detectBrowserLanguage: false
    }
  }
})
// restarts server with `NUXT_PUBLIC_I18N_DETECT_BROWSER_LANGUAGE=false`
await restoreConfig()
// restarts server with default env, which unsets previously set config

Do note that, if you don't restore/empty the runtime config, any following tests will be using the configured runtime config as well.

I'm sure there are changes to be made in this PR, please let me know what can be improved!

@danielroe danielroe requested a review from pi0 December 11, 2023 17:58
@danielroe danielroe added the enhancement New feature or request label Dec 11, 2023
import { snakeCase } from 'scule'
import { startServer } from './server'

const NUXT_ENV_PREFIX = 'NUXT_'
Copy link
Member

Choose a reason for hiding this comment

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

While not fully documented, it might be worth to not hardcode it (using it as default is fine) but rely on the nitro runtime config value. See nuxt/nuxt#23714

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I haven't seen that being used before, I've changed the function to accept an optional argument envPrefix.

... rely on the nitro runtime config value

It would be nice if we could retrieve which prefix is being used in the running fixture, I'm not sure how to do that. An alternative would be checking if nuxtConfig.runtimeConfig.nitro.envPrefix is present in the test context and using that.

Copy link
Member

@manniL manniL Dec 12, 2023

Choose a reason for hiding this comment

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

yeah, I think the latter would be the easiest, defaulting to NUXT_ otherwise. But an option/2nd optional arg is fine too for me

await startServer({ env })

// restore
return async () => startServer()
Copy link
Member

Choose a reason for hiding this comment

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

I am not too much fan of this API. setRuntimeConfig sounds like doing a runtime operation without full server restart and nitro useRuntimeConfig - when used correctly in context - reflects any runtime env changes as soon as they change without need of a manual server restart. If manual server restart is desired for any incompatibility, it should be opt-in IMO with something like setRuntimeConfig({}, { restart: true } (we could also automatically do it for well known keys such as baseurl)

Copy link
Member Author

Choose a reason for hiding this comment

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

Is the issue that it sounds too similar to useRuntimeConfig while not functioning in a similar way (trigger restart, not reactive), or that the server gets restarted either way?

The reason for restarting the server is that I don't know how (or if it is possible) to update the env variables of the running server process. It would be preferable to be able to handle/apply this without a server restart, if that is not possible I don't think opt-in restart makes much sense in my opinion.

I agree that it sounds similar to useRuntimeConfig, maybe a better name could be applyRuntimeConfig, overrideRuntimeConfig or overwriteRuntimeConfig?

Something else I considered would be something like this:

await withRuntimeConfig(config, async () => { /* tests that use `config` */ }, options)

So that restoring/resetting config always happens without additional action, I think having to restore it separately can easily be forgotten.

Either way, I'm open to suggestions 😅 I'm not aware of other ways to easily test runtime config support for module authors, so I think this is essential if we want more modules to reliably support it.

Copy link
Member

@pi0 pi0 Dec 13, 2023

Choose a reason for hiding this comment

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

The reason for restarting the server is that I don't know how (or if it is possible) to update the env variables of the running server process

An IPC signal can do it using process.send . Alternatively, we can export a dev middleware when starting nuxt in test mode to allow it. It is much cheaper than full-reload and also allows stateful testing when we need to see the effect of one even without for example losing connection. Please ping me if need any more insights on how to do this.

I agree that it sounds similar to useRuntimeConfig, maybe a better name could be applyRuntimeConfig, overrideRuntimeConfig, or overwriteRuntimeConfig?

If you are thinking this (as a set-and-restart) would be still a useful tool and faster to iterate, I would suggest to explicitly call it startServerWithRuntimeConfig as it does this exactly.

Copy link
Member Author

@BobbieGoede BobbieGoede Dec 15, 2023

Choose a reason for hiding this comment

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

An IPC signal can do it using process.send

Thanks for the hint!

I managed to communicate with the server process using process.send and am able to make changes to the process.env while keeping it running. Using a server route I can confirm that useRuntimeConfig(event) contains the updated values, unfortunately it doesn't seem to update in plugins (testing server side) before/after page reload.

Does overwriting runtime config variables with environment variables only work in server routes or should this also work elsewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe I'm on the wrong track entirely but it seems like things get a bit more complicated.. 😅 Please let me know if this sounds right or not.

1. Server routes
Runtime config can be overwritten by setting environment variables using an IPC signal.

2. Server-side plugins and such
Runtime config is frozen, overwriting requires restarting server with environment variables.

3. Client-side
Runtime config is reactive, can be changed using page.evaluate or something along those lines.

Copy link
Member

Choose a reason for hiding this comment

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

Does overwriting runtime config variables with environment variables only work in server routes or should this also work elsewhere?

We might need to (also) listen via a Nuxt plugin but seems so strange because last I recall Nuxt also uses direct import from Nitro utils...

Do you mind to push your latest changes? If feel too much different another (draft PR) might also be good so I can check locally somehow 👍🏼

Copy link
Member Author

Choose a reason for hiding this comment

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

I've been working on this locally inside the @nuxtjs/i18n repo as I already have tests for runtime config there, also I'm not sure what the intended workflow in this repo is but I find myself having to run dev:prepare and prepack before running tests/checking if the changed test utils work.

The code is a bit of a mess as I was exploring what worked and what didn't, I'll clean it up a bit and open a draft PR.

We might need to (also) listen via a Nuxt plugin ...

My implementation uses a Nuxt plugin to start listening for messages, is that what you mean?

... but seems so strange because last I recall Nuxt also uses direct import from Nitro utils...

When an H3Event is passed to useRuntimeConfig it does pick up the changes, otherwise it keeps using the original values. I think you describe the same issue of having to restart the server/Nuxt instance here: nuxt/nuxt#14330.

I'll link the draft PR here once I have it ready!

Copy link
Member

Choose a reason for hiding this comment

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

My implementation uses a Nuxt plugin to start listening for messages, is that what you mean?

Then you probably can try swapping/adding a Nitro plugin. It runs before and also applies for server logic.

Copy link
Member Author

Choose a reason for hiding this comment

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

@pi0
Here's the draft PR #670

@danielroe danielroe changed the title feat: add setRuntimeConfig module utility feat(e2e): support env option for startServer Jan 17, 2024
@danielroe danielroe merged commit b693232 into nuxt:main Jan 17, 2024
3 checks passed
This was referenced Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants