Don't validate example/ for dependencies

jonasfj commented 5 years ago

When publishing a package we currently validate that files don't depend on packages that aren't referenced. This also goes for files in example/.

@nex3 makes the case in https://github.com/dart-lang/pub/issues/1813#issuecomment-369055089 that:

this is intended behavior,
packages shouldn't contain large demos/examples,
example/ is only for showing how the API works, and,
example/ is distributed with the package, thus, should be kept small.

On the other hand,

Of the 1000 most popular packages 289 contain a pubspec.yaml somewhere in the example/ folder.
package layout convention seems to hint that one can use multiple files and a example/lib/ folder... indeed this suggests you should be shipping a "standalone example program" along with your package.
small API examples are often present in the README.md or in API docs,
test/ is also distributed with the package (so why not demo/example applications),

I propose that we remove 'example' from the checks below, and update the package layout convention to clearly state that example/ can be have it's own pubspec.yaml.

https://github.com/dart-lang/pub/blob/154a1419955df0e19a5c20aeeff220fbb5ec6efc/lib/src/validator/strict_dependencies.dart#L97-L107

Thoughts: @nex3, @munificent, @kevmoo, @mit-mit, @sigurdm?

We can also update the package layout convention to say that example/ is not an independent package and should not contain a pubspec.yaml. But we should clarify our intent.

/CC @amirh, @iskakaushik, @isoos,

amirh commented 5 years ago

Also note that flutter create -t plugin currently creates a sample app in the example/ subfolder.

It seems like sometimes a non-trivial app is required to properly demonstrate and/or test plugins, additionally in flutter/plugins we are starting to rely on e2e tests that drives standalone Flutter apps.

cc @collinjackson

kevmoo commented 5 years ago

The Flutter folks took some liberties w/ /example without talking to the folks who maintain pub.

I must say, I'm not a fan of having nested packages – makes a lot of things hard.

kevmoo commented 5 years ago

By nested: having a directory w/ pubspec.yaml under another directory w/ pubspec.yaml.

The dart core folks have tried to use mono repos here. Where we have examples as sister directories.

amirh commented 5 years ago

It looks like the example folder in the flutter plugin template dates back to this PR https://github.com/flutter/flutter/pull/9076

Maybe the reviewers have some more context as for why it was done this way vs. the project structure that @kevmoo suggests? (tagging reviewers of that PR: @Hixie @mit-mit @cbracken @devoncarew @danrubel )

kevmoo commented 5 years ago

To be clear: we know why they went w/ that structure. It allows a full example experience for any package to be shipped w/ a package. It has a lot of value.

It also adds complexity to our analysis and pub tools. We need to work to figure out the best way forward.

jonasfj commented 5 years ago

To be clear: I don't really care much what consensus we reach. But if pub has an opinion about the contents of example/, then we should be consistent about it.

Hixie commented 5 years ago

I didn't realise there was any controversy here. The example directory is based on the advice in https://www.dartlang.org/tools/pub/package-layout#examples and adding a pubspec.yaml there is necessary to demonstrate some of the things we are trying to demonstrate in the examples. If there's a different best practice I expect we would be open to it, assuming it is understandable to our users.

collinjackson commented 5 years ago

There is one example that I can think of in the Flutter first-party plugins repo, firebase_auth, where we are trying to keep the core package's pubspec.yaml dependencies as minimal as possible but show how the plugin can be combined with another package (Google Sign In) in the example.

Recently we've been writing some Flutter plugin tests that require flutter_driver in the example app but not in the main package. It would be more intuitive (to me) to be able to keep the flutter_driver dependency confined to the example. Since flutter_driver isn't versioned through pub, it doesn't really matter though. https://github.com/flutter/plugins/blob/master/packages/cloud_functions/example/test/cloud_functions.dart

natebosch commented 5 years ago

where we are trying to keep the core package's pubspec.yaml dependencies as minimal as possible

I think it would be reasonable to have these in dev_dependencies for the "parent" package. The only downside, and I think it's small, is this could lead to restricting the pub solve during development and cause the single pub get to fail, where it might succeed if the deps were in separate packages.

One thing I would worry about if we force away from this situation is if we're making it any harder to build one of these examples as a sub-directory of a package rather than as it's own package.

I would imagine it's common practice to jump into the example directory an flutter build? Is there a reasonable way to replace that workflow if it doesn't have its own pubspec and .packages? If the parent package did have all the right deps in dev_dependencies would a flutter build be able to build the example or would it fall down on example/lib/main.dart not being lib/main.dart?

sigurdm commented 5 years ago

A few thoughts:

Having the example in another folder in a mono_repo makes it harder to ensure that the example you are looking at corresponds to a given version on pub. I think we should allow having nested (example) packages and fix tools where this causes problems.
Ideally pub get would download a package containing only /lib (and defined assets), and pub activate would also get /bin and perhaps /web. It makes no sense to download tests, examples, docs and tools for all packages. All these are not very useful or discoverable from the pub cache. I don't have a full overview of ways we are using/exposing stuff outside here, but perhaps we can still pursue this direction. A new command pub try would download and run examples.

jakemac53 commented 5 years ago

Having the example in another folder in a mono_repo makes it harder to ensure that the example you are looking at corresponds to a given version on pub.

What makes this any different from a nested package with its own pubspec? In both cases you have to decide either to do a path dependency override (best option imo), or put some dependency constraint.

I think we should allow having nested (example) packages and fix tools where this causes problems.

What value does a nested example package with a pubspec provide over using dev_dependencies in the main package to add any extra deps required in the examples?

Ideally pub get would download a package containing only /lib (and defined assets), and pub activate would also get /bin and perhaps /web. It makes no sense to download tests, examples, docs and tools for all packages. All these are not very useful or discoverable from the pub cache.

This has always been a bit weird - but unfortunately at this point all kinds of tools reach outside of the lib directory because they can and don't have any other choice. We might be able to get away with blacklisting a couple directories like test, but I don't think we can use a whitelist.

Some examples of tools that do this today:

flutter allows "assets" in any arbitrary directory in the package and will create asset bundles from those (you do have to list them in the flutter assets section in your pubspec though).
build_runner reads build.yaml and pubspec.yaml files from dependencies
dev_tools publishes the built web app under a build directory and serves from that using its binary under bin.

There are a lot of other examples that make this difficult unfortunately.

A new command pub try would download and run examples.

How would it know how to "run" the examples?

sigurdm commented 5 years ago

What makes this any different from a nested package with its own pubspec? In both cases you have to decide either to do a path dependency override (best option imo), or put some dependency constraint.

The nested package is published together with the containing package, and can be presented by the pub site.

I agree that path dependency overrides is the way to go

What value does a nested example package with a pubspec provide over using dev_dependencies in the main package to add any extra deps required in the examples?

I think dev_dependencies not used by the main library but only by an example are confusing obscuring the true structure of your project and also makes the example harder to understand/replicate outside the package.

Also showing a true client pubspec is a nice way of demonstrating/testing how including library works.

To turn the question on the head: What do you get by not having a pubspec for your example? (This is an honest question, I might be overlooking a lot of issues - but I have not met a lot of problems when developing flutter plugins with examples).

There are a lot of other examples that make this difficult unfortunately. Blacklisting test and example sounds useful...

My (poorly expressed) point was, that the size of the downloaded package should not be what limits the expressiveness of examples.

How would it know how to "run" the examples?

I have not thought it through very much, but perhaps there can be a number of modes: for flutter a flutter package it would do flutter run in the example folder, for a web-sample it would do webdev serve for an command example it would run it. It would require declaring the type of the example somewhere.

This is all dreaming inspired by https://github.com/CocoaPods/cocoapods-try - maybe it's mostly relevant for the flutter case.

jakemac53 commented 5 years ago

The nested package is published together with the containing package, and can be presented by the pub site.

Definitely true and a good point - mono_repo examples in sibling directories are only really discoverable if you are looking at the repo itself. I suppose you could somehow link in examples from other directories (via some pubspec config etc) but that sounds overly complicated compared to just using example.

Also showing a true client pubspec is a nice way of demonstrating/testing how including library works.

I think that is a fair point as well - having an example package that you can basically just copy/paste anywhere and use has value (although then maybe a path dependency isn't the right thing?).

To turn the question on the head: What do you get by not having a pubspec for your example?

If the example is a package on its own, then in build_runner/webdev it can't share any work with the root package. It is treated as a completely separate project and it starts from scratch. You also have to cd into the example directory and run from there.

Instead if you leave it as part of the root package you can do webdev serve example, and it will share any work that was already done (compiling dependencies, the root package itself, etc). You can also serve the entire package at once (all its tests, web dir, and example dir) this way.

There are also the issues around tooling - build_runner today is example of one of the tools that doesn't work by default with the nested package example. It will work if the nested package is in the root package dependency graph, but if it isn't then it would treat the nested package as part of the root package, which leads to compilation errors (for missing deps). This could be fixed, but I am wary of simply omitting all directories with a pubspec.yaml from the root package, and I don't like special casing the example dir. The workaround today in build_runner is to manually exclude the example dir from your default target.

sigurdm commented 5 years ago

I think for flutter examples having its own pubspec is necessary. It might need assets. I don't think we want to include them in the parent pubspec (that would require dev_assets - that sounds scary).

sigurdm commented 5 years ago

Instead if you leave it as part of the root package you can do webdev serve example, and it will share any work that was already done (compiling dependencies, the root package itself, etc).

This is the same situation for any other client of the root package. I don't think we should optimize for efficient compilation specifically for examples. If we can share some work we should do it in a way, that the optimization should also apply for any user of the root package.

You can also serve the entire package at once (all its tests, web dir, and example dir) this way.

I think this sounds confusing. Why would I serve a library? My experience using webdev is limited, but IMO a package should either be a library or an app. If we start being both the semantics become unclear.

When do you serve tests?

There are also the issues around tooling

Yes - tooling would need to learn nested packages. But given that a quarter of top 1000 packages already break the convention I think it looks like a worthy investment.

If we are right that we can just ignore any subdirectory with a pubspec it sounds not too hard. (There might be cases I cannot think of).

jakemac53 commented 5 years ago

I think for flutter examples having its own pubspec is necessary. It might need assets. I don't think we want to include them in the parent pubspec (that would require dev_assets - that sounds scary).

That is a nice concrete reason that an example dir would want its own pubspec.

I think this sounds confusing. Why would I serve a library? My experience using webdev is limited, but IMO a package should either be a library or an app. If we start being both the semantics become unclear.

All libraries should (ideally) have at least tests and examples, while applications should (ideally) have at least test and a web directory (or in the case of flutter an app under lib).

So while yes probably having example, test, and web would be uncommon - having two of those three should be typical.

When do you serve tests?

For the web we support serving the tests and running them directly in the browser (without going through the package:test runner which is clunky for debugging).

If we can share some work we should do it in a way, that the optimization should also apply for any user of the root package

It is a much gnarlier problem to try to handle this generally, while keeping it part of the main package works out of the box today. The main problem being that compiled artifacts depend on the full transitive dependency solve, so its very difficult to pre-build anything on pub etc. Local code generation into dependencies also makes this even more difficult.

jonasfj commented 5 years ago

Another way at this is that pub generally allows anything it doesn't understand. Whether it be unknown fields in the pubspec or arbitrary files included in the package.

This leads to unfortunate cases when there is a typo... Or people don't follow conventions. Files included by accident. But its also very powerful, in that allows conventions to organically form.

Right now, example/ is used by pub.dartlang.org and people browsing source repositories. We have no tools kicking off a dartpad from an example, we can't programmatically run examples, etc.

So why should pub care what is in example/? If I called the folder demo/ then nobody would care.

The argument I suppose I'm reluctantly making is that if pub cares about the content. It should do so for a reason. (Not sure of that makes any sense)

jonasfj commented 5 years ago

Having discussed this with a lot of people IRL, we've reached the decision that:

pubspec.yaml in examples/ is a common convention,
we should remove any ambiguity around it being permitted.

If anyone strongly disagrees now is the time to speak up. Otherwise, we'll move forward with changes to documentation and pub client.

jonasfj commented 5 years ago

This was fixed in https://github.com/dart-lang/pub/pull/2107

dart-lang / pub

Don't validate example/ for dependencies #2101