Skip to content

feat: Add runtime config support #254

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

Merged
merged 18 commits into from
Jan 21, 2021

Conversation

julien1619
Copy link
Contributor

This PR adds a new configuration key: publicRuntimeConfigKey, it defaults to sentry when publicRuntimeConfig exists.

It fixes #202, and inspired by #203, thanks to @Ledarath

@codecov
Copy link

codecov bot commented Nov 26, 2020

Codecov Report

Merging #254 (2cd8968) into master (4a57102) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #254   +/-   ##
=======================================
  Coverage   55.55%   55.55%           
=======================================
  Files           1        1           
  Lines          27       27           
  Branches        8        8           
=======================================
  Hits           15       15           
  Misses          9        9           
  Partials        3        3           
Impacted Files Coverage Δ
lib/module.js 55.55% <100.00%> (ø)

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 4a57102...2cd8968. Read the comment docs.

Copy link
Member

@rchl rchl left a comment

Choose a reason for hiding this comment

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

Missing:

  • documentation
  • similar change in plugin.lazy.js
  • What about server-side? Even though on server-side options are "runtime", the user might want to use the runtime config for both server and client-side rather than having to specify them in both places.

@julien1619
Copy link
Contributor Author

Thanks for the review, I'll do that tomorrow.

@julien1619
Copy link
Contributor Author

julien1619 commented Nov 27, 2020

@rchl I did all the requested changes.

  • Add documentation
  • Replace deepmerge and all Object.assign by lodash.merge
  • Add Runtime Config to lazy mode and server-side
  • Restore serialize instead of JSON.stringify

@julien1619 julien1619 requested a review from rchl November 27, 2020 11:10
@julien1619
Copy link
Contributor Author

I've tested it on my site and there is a bug in the config merge, I'll investigate tomorrow. For now I'll set the draft mode on this Pull Request.

@julien1619 julien1619 marked this pull request as draft November 27, 2020 17:56
@julien1619 julien1619 marked this pull request as ready for review November 28, 2020 00:00
@julien1619
Copy link
Contributor Author

It works now. @rchl can you review it again? Thanks!

@julien1619
Copy link
Contributor Author

@rchl Just did a rebase onto master. Is there something blocking this PR from being merged? Thanks!

@rchl
Copy link
Member

rchl commented Dec 10, 2020

Just lack of time but will try to handle soon

@julien1619
Copy link
Contributor Author

@rchl We did all the changes requested. The publicRuntimeConfig can now be used to override config, clientConfig and serverConfig (Thanks to @Ledarath ) Can you review it? Thanks!

@rchl
Copy link
Member

rchl commented Jan 20, 2021

Would you be able to enable the option that allows maintainers to push to this branch? I could do some minor changes myself instead of the long way with comments and turnarounds.

@julien1619
Copy link
Contributor Author

Done! I invited you directly, the option "Allow edits from maintainers" wasn't available.

rchl added 2 commits January 20, 2021 23:30
Since, if in the future we also want to support privateRuntimeConfig, it
IMO makes sense to have the same key for both.
@rchl
Copy link
Member

rchl commented Jan 20, 2021

OK, I've done some changes. You can have a look through the commits I've added and check if it looks good to you.

@julien1619
Copy link
Contributor Author

LGTM 👍

@rchl rchl merged commit 7f8b373 into nuxt-community:master Jan 21, 2021
@rchl rchl deleted the feature/runtime-env branch January 21, 2021 21:25
@rchl
Copy link
Member

rchl commented Jan 21, 2021

Thanks!

@samturrell
Copy link

Sorry to jump in on this late. To confirm, the dsn still has to be provided at build time and will be baked into the built code? There's no way of providing this as a runtime config value?

@rchl
Copy link
Member

rchl commented Jan 22, 2021

A bit weird maybe but you can probably set any dsn in build-time settings and override it through runtime settings.

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.

Plugin not using runtime variables from $config
4 participants