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

Split out directDependencies and devDependencies in pub deps #4383

Merged
merged 8 commits into from
Sep 25, 2024

Conversation

sigurdm
Copy link
Contributor

@sigurdm sigurdm commented Sep 12, 2024

Fixes #4330

Comment on lines 130 to 131
if (isRoot)
'dev_dependencies': currentPackage.devDependencies.keys.toList(),
Copy link

Choose a reason for hiding this comment

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

Should this be devDependencies? I don't know about the rest of this file, but files like package_config.json seem to use camel case (for ex. rootUri)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question - in pubspec.yaml we use dev_dependencies - but json guidelines generally suggest camelCase.

@jonasfj do you have an opinion?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know, I guess using devDependencies is the most consistent with other JSON outputs we have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@jonasfj
Copy link
Member

jonasfj commented Sep 12, 2024

This seems like a nice solution, it's probably even backwards compatible for people who never expected there to be a devDependencies property.

@sigurdm
Copy link
Contributor Author

sigurdm commented Sep 12, 2024

This seems like a nice solution, it's probably even backwards compatible for people who never expected there to be a devDependencies property.

Except we remove the dev-dependencies from the dependencies property... which might break something...

@DanTup
Copy link

DanTup commented Sep 12, 2024

Some additional thoughts:

  • Is this JSON format specified anywhere that should be updated?
  • Should there be some kind of protocolVersion field that could be increased with changes like this (that makes it easier for compatibility to know which version of the JSON we have without having to know which SDK versions had it)
  • Can we be certain that devDependencies is an empty list (for root packages) when there are none? (eg. can I use devDependencies being missing as an indicator to read dependencies the "old" way)?

@sigurdm
Copy link
Contributor Author

sigurdm commented Sep 12, 2024

Is this JSON format specified anywhere that should be updated?

No it is not. Perhaps it should be in https://dart.dev/tools/pub/cmd/pub-deps .

Should there be some kind of protocolVersion field that could be increased with changes like this (that makes it easier for compatibility to know which version of the JSON we have without having to know which SDK versions had it)

Yeah - perhaps this is the right way to do it. I can add a "version": 2 field.
I guess it will not prevent any breakage, because nobody checks this field yet.

Can we be certain that devDependencies is an empty list (for root packages) when there are none? (eg. can I use devDependencies being missing as an indicator to read dependencies the "old" way)?

As it is currently written, then yes - maybe we should add a comment about it.

@sigurdm
Copy link
Contributor Author

sigurdm commented Sep 12, 2024

@sigurdm
Copy link
Contributor Author

sigurdm commented Sep 12, 2024

What if the "devDependencies" listed all "dev_dependencies" and "dependencies" continued to list deps+dev_deps ... that would be backwards compatible, but slightly confusing...

@DanTup
Copy link

DanTup commented Sep 12, 2024

Yeah - perhaps this is the right way to do it. I can add a "version": 2 field.
I guess it will not prevent any breakage, because nobody checks this field yet.

It will make it easier to know whether dependencies includes dev_dependencies though (the empty list also works, but versioning and a changelog/description somewhere would be ideal).

Oh - I already found someone we are breaking (flutter tools):
...
What if the "devDependencies" listed all "dev_dependencies" and "dependencies" continued to list deps+dev_deps ... that would be backwards compatible, but slightly confusing...

Personally, I think it would be better to make the break and update Flutter.. It's a trivial change and the Flutter versions are tied to pub so there's no compatibility issue there as long as the Flutter change is made before Pub rolls in. However, I am assuming there's probably no other consumers (or if there are, this wouldn't break them too badly) and I consume a lot of consuming APIs so feel like less confusion is always better 😄

@sigurdm sigurdm changed the title Split out dev_dependencies in pub deps Split out directDependencies and devDependencies in pub deps Sep 24, 2024
@sigurdm
Copy link
Contributor Author

sigurdm commented Sep 24, 2024

We have ended up on having

  • dependencies with the old semantics
  • directDependencies with the "dependencies" from the pubspec
  • devDependencies with the "dev_dependencies" from the pubspec (if the package is a root package)

Reasoning:

We chose to let backwards compatibility rule over neatness because

  • the --json output is mainly for machine consumption anyways, and
  • is most useful if we can avoid it breaking ever.

We chose not to have a version field in the output because:

  • It is not clear whom it would help.
  • It would still break existing clients if we removed/changed the old field

We chose not to have a --json2 or similar because

  • that is just as ugly, and requires branching on version up front

PTAL

@DanTup
Copy link

DanTup commented Sep 24, 2024

We have ended up on having

  • dependencies with the old semantics
  • directDependencies with the "dependencies" from the pubspec
  • devDependencies with the "dev_dependencies" from the pubspec (if the package is a root package)

This sounds good to me :-)

We chose not to have a version field in the output because:

  • It is not clear whom it would help.

I don't think it's necessary now, but with the previous version it would've been a good indicator for updated clients to know if dependencies had devDependencies excluded. However, if we could rely on an empty list for devDependencies, that would've also been an indicator.

If you think this is pretty complete now, let me know and I can try updating Dart-Code to use JSON in this format and see if I hit any issues.

Thanks!

@DanTup
Copy link

DanTup commented Sep 24, 2024

@sigurdm is there a way I can include this in an SDK build locally (or otherwise run this version locally)? I tried updating my DEPS to use a hash from this PR and running gclient sync, but it fails to find this revision - presumably because it's in your fork and not the main pub repo, but I can't find where the Git repo URL is recorded.

Edit: nm, I found it also in DEPs :) This worked:

  Var("dart_root") + "/third_party/pkg/pub":
      "https://github.com/sigurdm/pub.git" + "@" + Var("pub_rev"),

@DanTup
Copy link

DanTup commented Sep 24, 2024

I have changes in Dart-Code working using this PR. I fixed a couple of other bugs in the dependencies tree while working on it, so if it's not likely to change again, I'd like to merge those changes (before the next release at the end of the month). If things might change further, then I'll try to unstitch some of those fixes from this change.

@sigurdm sigurdm requested a review from jonasfj September 25, 2024 07:29
@sigurdm
Copy link
Contributor Author

sigurdm commented Sep 25, 2024

If @jonasfj is happy, then I think we'll stay with this.

I don't think it's necessary now, but with the previous version it would've been a good indicator for updated clients to know if dependencies had devDependencies excluded. However, if we could rely on an empty list for devDependencies, that would've also been an indicator.

You can use the presence of "directDependencies" to distinguish between new and old.

Copy link
Member

@jonasfj jonasfj left a comment

Choose a reason for hiding this comment

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

r+

@sigurdm sigurdm merged commit 23e3d4f into dart-lang:master Sep 25, 2024
23 checks passed
@DanTup
Copy link

DanTup commented Sep 25, 2024

Awesome, thanks! :)

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.

Pub workspaces: "pub deps" returns information for the whole workspace and not the current package
3 participants