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

Add new preferences module #9115

Merged
merged 19 commits into from
Nov 29, 2023
Merged

Add new preferences module #9115

merged 19 commits into from
Nov 29, 2023

Conversation

natemoo-re
Copy link
Member

@natemoo-re natemoo-re commented Nov 16, 2023

Changes

  • Resolves PLT-1200, related to experimental.devOverlay
  • Adds a new "user preferences" module. Whereas the astro.config.mjs file manages configuration for a specific project on any machine, user preferences manage configuration for a specific user (machine).
    • By default, user preferences are scoped to a specific project and stored in the .astro/settings.json file.
    • User preferences can also be stored globally and apply to every Astro project. They are stored in an OS-specific location.
  • Why? The goal here is to allow users to toggle the dev overlay on/off in a way that doesn't affect every other person that works on the project. Since we already automatically .gitignore the .astro/ directory, this is a natural place to put settings for an individual user.
  • Adds a new astro preferences CLI command, designed to mirror the pnpm config command.
# Disable the dev overlay for the current user in the current project
npm run astro preferences disable devOverlay
# Disable the dev overlay for the current user in all Astro projects on this machine
npm run astro preferences disable devOverlay --global

# Check if the dev overlay is enabled for the current user
npm run astro preferences reset devOverlay

Open Questions

  • Should this module be in astro or a separate package? I could see this module being used in @astrojs/telemetry and create-astro to configure certain default behavior.
    • Resolution: We might want to do this in the future, but we don't have to do it now.
  • Should the CLI be called via astro config? I can see that being hard to document. I considered astro prefs or astro settings but both felt weird.
    • Resolution: astro preferences it is! config would have been confusing to users because of the astro.config.mjs file and settings would have been confusing for maintainers because there is an existing AstroSettings that means something else.
  • This is a new feature that increases the scope of the dev overlay. How do we message it as experimental? Should we flag the CLI command as astro experimental-config?
    • Resolution: YOLO.

Testing

Tested manually because I'm not sure how pipe the mocked fs through the system just yet

Docs

#5512 and added to the Astro 4.0 breaking changes tracker

Copy link

changeset-bot bot commented Nov 16, 2023

🦋 Changeset detected

Latest commit: fb9e046

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review semver: minor Change triggers a `minor` release labels Nov 16, 2023
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This PR is blocked because it contains a minor changeset. A reviewer will merge this at the next release if approved.

@Princesseuh
Copy link
Member

Should this module be in astro or a separate package? I could see this module being used in @astrojs/telemetry and create-astro to configure certain default behavior.

I could definitely see this in a separate package, I notable can very easily see create-astro being interested in it (it could be neat if it remembered your usual settings? People don't start up projects all the time for sure, but maybe if you're always using strictest, it'd be cool if it defaulted to it, no idea.)

Should the CLI be called via astro config? I can see that being hard to document. I considered astro prefs or astro settings but both felt weird.

No particular thoughts, but I assume we don't want users to confuse it with astro.config.mjs, so maybe it needs a separate name?

This is a new feature that increases the scope of the dev overlay. How do we message it as experimental? Should we flag the CLI command as astro experimental-config?

Since the dev overlay is stable in like two weeks, maybe we just yolo it? I don't see us relying on this module for othe rthings soon anyway.

@FredKSchott
Copy link
Member

FredKSchott commented Nov 16, 2023

If we're bikeshedding on command name/API:

  • I agree astro config sounds a lot like "astro config file" or "astro.config.js" and the distinction between project-level and user/machine-level would get totally lost. Would love to find a different name for it.
  • I like astro settings and more importantly the idea that "settings" is a term we use for global machine-level configuration. preferences or another word could work as well, it really just comes down to us choosing a word that makes sense AND we can say "X means global settings" in our terminology. +1 on whatever Nate decides here (just no abbreviations please!).
  • I dislike the complexity of "devOverlay.enabled" and true|false in the CLI because it feels like a JSON implementation detail that's leaking. I'd prefer the following API:
    • astro settings [enable|disable] [telemetry|overlay]
    • astro settings set future-complex-item-name "value"
    • astro settings unset future-complex-item-name
    • astro settings list
    • to be clear, only the first example in my list needs to be built today! Just thinking through this a bit more than I normally would this early since we're thinking about this as a more long-term, reusable API.
    • @natemoo-re if you intentionally like the idea of exposing the JSON settings interface intentionally (astro settings [enable|disable] devOverlay.enabled) because you imagine the user interacting with this via JSON at some point, I'm not blocking / I see the argument for it!

@ematipico
Copy link
Member

I firmly believe that we shouldn't provide a --global flag, especially because we suggest users to not install astro globally.

@Princesseuh
Copy link
Member

I firmly believe that we shouldn't provide a --global flag, especially because we suggest users to not install astro globally.

I don't really understand how the global config is related to installing Astro globally, what's the relation?

@natemoo-re
Copy link
Member Author

Thank you for the feedback @FredKSchott! I believe I've addressed all of that now.

  • astro settings would have been a good name, but we have an existing AstroSettings object throughout the codebase that means something different. To avoid internal confusion, I settled on astro preferences.
  • I agree with your thoughts on [name].enabled true/false feeling weird! I had originally considered a design like you suggested but wasn't sure if it was the right move. Since you brought it up, I think a subcommand interface will be much easier to interact with.
    • astro preferences get [key] will print the current value, or let you know the default if it's not set
    • astro preferences set [key] [value] will set the current value
    • astro preferences enable [key] will set a ${key}.enabled value to true. This matches the existing telemetry command.
    • astro preferences disable [key] will set a ${key}.enabled value to false. This matches the existing telemetry command.
    • astro preferences reset [key] will delete the value. delete is an alias. This matches the existing telemetry command.
    • astro preferences list will list all your current preferences. Using the --json flag will give a machine-readable output.

@natemoo-re
Copy link
Member Author

@ematipico I understand where you're coming from, but the ability to set a preference for all Astro projects is one of the explicit goals. I don't think users will confuse a --global flag with installing astro globally, but I could be wrong.

For some additional context: the preferences CLI command mirrors what the existing astro telemetry [disable|enable|reset] command does, except --global was always implied in that case. The eventual goal is to unify these two commands under one astro preferences command, so we're going to need --global to make that happen. I also hope to support personalized defaults for create-astro in the future (also global).

@ematipico
Copy link
Member

I firmly believe that we shouldn't provide a --global flag, especially because we suggest users to not install astro globally.

I don't really understand how the global config is related to installing Astro globally, what's the relation?

Having a local and a global configuration is a very risky business, because a single change in the configuration, even the addition of a new property, would mean that a previous version wouldn't be able to understand it.

If this single change goes into the global configuration, the rest of the projects won't work anymore, unless they are upgraded. It would make DX a living hell.

I'm not sure we should allow a global configuration.

@natemoo-re
Copy link
Member Author

I'm still not sure I understand your specific concerns. The intention for this feature is to support global user preferences that impact every Astro project. Telemetry already works this way—if a user opts out of telemetry, every Astro project needs to respect that preference. Semver and backwards-compatibility are a requirement with the global preferences as they are with any feature.

@matthewp
Copy link
Contributor

To handle versioning of config file changes I've seen done by having a "version" field in the config file. Since this file is edited by a CLI and not by hand, I think that handles the concern well.

@ematipico The UX here of "I want this disabled in all projects" seems perfectly valid as a goal. Would you agree? If so, is there any other way to achieve that goal than something like this?

Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Docs look great for the changeset, Nate! We'll just need something added to the CLI reference page in docs itself!

@ematipico
Copy link
Member

ematipico commented Nov 21, 2023

I don't know how much we plan to rely on global settings in the future, so please take my takes with a grain of salt.

To handle versioning of config file changes I've seen done by having a "version" field in the config file. Since this file is edited by a CLI and not by hand, I think that handles the concern well.

I am aware of it, but this won't fix our issues.

Here's a practical example. You add a global ~/.astro/settings file that will look like this (it's pseudo code). This property has a typo. This property is released in version 3.7.0.

version = 1
devOvelay = true

Upon revisiting the file, we discovered the issue and published a patch in 3.7.1. Now, 3.7.1 won't be able to understand the contents in ~/.astro/settings because we had a typo. It will create a new file from scratch, I suppose:

version = 1.1
devOverlay = true

Now all projects with version 3.7.0 can't understand the new config file.

Also, imagine these issues on minor releases or major releases of astro. We can't force users to upgrade ALL their projects.


The way I see it, the only way to land this feature without repercussions, is to make sure that Astro can understand ALL the versions of ~/.astro/settings and provide a way to transition from one way to another, without changing the actual file. This isn't a solution though, it just mitigates the problem.

@FredKSchott
Copy link
Member

To @ematipico's concern, if Nate committed to long-term full backwards compatibility, would that address your concern? Ex: in your scenario above, the "fixed" version of Astro would write to both devOvelay and devOverlay indefinitely.

For context, this is not a module that we should expect a lot of usage and changes in. To look at its history, you could say that we introduced the first preference for telemetry in Astro 1.0, and now the second preference for the dev overlay in 4.0. I don't think there's any reason to think that this will accelerate, and if it did I imagine we could revisit this system in the future without being too blocked.

My main point would be that if your concern is with the larger concept of global configuration, that's probably a larger conversation given that we already have the concept of global configuration in Astro today powering the global telemetry opt-out.

@matthewp matthewp changed the base branch from main to next November 22, 2023 12:55
@natemoo-re
Copy link
Member Author

natemoo-re commented Nov 22, 2023

Thanks for the elaboration @ematipico and for chiming in @FredKSchott! I understand the concern and agree that backwards-compatibility is essential for this feature. Changes to the configuration options would be handled with the same concern for proper semantic versioning as the rest of astro.

packages/astro/src/cli/preferences/index.ts Outdated Show resolved Hide resolved
packages/astro/src/cli/preferences/index.ts Outdated Show resolved Hide resolved
packages/astro/src/vite-plugin-astro-server/route.ts Outdated Show resolved Hide resolved
packages/astro/src/cli/preferences/index.ts Outdated Show resolved Hide resolved
@natemoo-re natemoo-re merged commit 3b77889 into next Nov 29, 2023
14 checks passed
@natemoo-re natemoo-re deleted the feat/dev-overlay-global branch November 29, 2023 18:43
This was referenced Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants