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

Expose additional properties. #1816

Open
schultek opened this issue Sep 8, 2024 · 9 comments
Open

Expose additional properties. #1816

schultek opened this issue Sep 8, 2024 · 9 comments

Comments

@schultek
Copy link

schultek commented Sep 8, 2024

It would be very helpful to expose additional properties that are defined in the pubspec as a Map<String, dynamic>.


Context:

With the Jaspr framework there are additional configuration options developers can set in the puspec under the jaspr key. This is similar to how Flutter has some configuration under the flutter key that is already supported by this package.

I'm depending on another package (custom_lint) that uses pubspec_parse to provide the pubspec values, and I have no other way to access these values.


I'm happy to make a PR for this.

@kevmoo

@kevmoo
Copy link
Member

kevmoo commented Sep 8, 2024

Great idea @schultek. I've asked some of the pub folks (I'm just an honorary pub folk) about this.

I think...if we want to support arbitrary tools reading pubspec, we should namespace things by the package name.

So maybe

dependencies:

pkg_config:
  my_pkg:
  my_pkg2:

Or similar to avoid conflicts. Kind of like the .dart_tool directory

This 147% a brainstorm right now. Let's get into the work week a bit and see what others think...

@sigurdm
Copy link
Contributor

sigurdm commented Sep 9, 2024

I believe the pubspec.yaml is open-ended by design - and people have been putting all kinds of stuff there that is not read by the pub client (eg. flutter). I'm not sure I always agree with this design, as it can make it harder for us not to step on other people's toes.

But it has been working out ok until now...

I think it would be reasonable for pubspec_parse to expose the raw YamlMap of the pubspec.
Maybe @jonasfj has opinions...

I think...if we want to support arbitrary tools reading pubspec, we should namespace things by the package name.

We have been discussing something similar here:

dart-lang/pub#3917

@jonasfj
Copy link
Member

jonasfj commented Sep 9, 2024

I think...if we want to support arbitrary tools reading pubspec, we should namespace things by the package name.

Yes,

But also as @sigurdm says pubspec.yaml has an open-ended design.

That said, I think: Encouraging people to custom keys under config.<packageName> is not a bad idea.


In general, I'm not entirely convinced that asking people to put custom keys in pubspec.yaml is a good idea.

With package:extension_discovery we've encouraged people to use extension/<targetPackage>/config.yaml for supplying options to be read by <targetPackage>.

That said, I can see that if you want to configure stuff in the root package, that might not be convenient.

But if your package is published to pub.dev, using pubspec.yaml might not be an ideal place to put custom keys. We have limits on the size of the pubspec.yaml, and the pubspec for all versions is included in the version listing API used by the pub client.

@schultek
Copy link
Author

schultek commented Sep 9, 2024

Thanks for discussing this.

As for my needs, it really doesn't need to be that complex.

I think it would be reasonable for pubspec_parse to expose the raw YamlMap of the pubspec.

Yes just that would be great.

Other ways or constraints won't help me anyways, as:

  1. this is about reading config in user projects, not packages published to pub
  2. the package that reads the config (jaspr_lints) is going to be an analyzer plugin and has a different name as the configuration property (jaspr)

In general, I'm not entirely convinced that asking people to put custom keys in pubspec.yaml is a good idea.

Flutter is doing that for years. I'm just following de-facto standards.

@jonasfj
Copy link
Member

jonasfj commented Sep 9, 2024

Disclaimer: I don't have fully formed thoughts here.

Flutter is doing that for years. I'm just following de-facto standards.

Yeah, and pubspec.yaml was designed with this in mind. Just please attempt to use a top-level property we don't want to use in the future 🤣

In hindsight, maybe it would have been better forbid custom top-level keys, and require that all custom keys are under something like:

dependencies:
  retry: ^3.0.0
config:
  retry:
    default_retries: 5

I don't think we can or should attempt to force that now. But we could establish a convention that custom keys in pubspec.yaml should be under config.<packageName> -- that way we reduce the risk of conflicts (and pubspec_parse could expose config as Map<String, Map<String, Object?>>).

I'm guessing we should formalize such a convention in: https://dart.dev/tools/pub/pubspec

And maybe augment it with a section that suggest using package:extension_discovery -- or give up on that idea, and make a package:config_discovery that does same as package:extension_discovery but reads from config.<targetPackage> in pubspec.yaml instead.

@jonasfj
Copy link
Member

jonasfj commented Sep 9, 2024

the package that reads the config (jaspr_lints) is going to be an analyzer plugin and has a different name as the configuration property (jaspr)

If jaspr_lints read from config.jaspr then yes, you could argue that it technically violates the convention. But honestly, so long as you own both package names who cares. The point is to reduce the risk of conflicting top-level names.

@schultek
Copy link
Author

schultek commented Sep 9, 2024

jaspr_lints reads from jaspr. I don't plan to move away from the top-level key.

@jonasfj
Copy link
Member

jonasfj commented Sep 10, 2024

jaspr_lints reads from jaspr. I don't plan to move away from the top-level key.

There is no harm from it either, jaspr isn't going to conflict with any feature we build in pub 🤣

@mosuem mosuem transferred this issue from dart-lang/pubspec_parse Dec 20, 2024
@Levi-Lesches
Copy link

This would be helpful to provide temporary support for new features or those that don't already have support, like:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants