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

Restructure library #2

Merged
merged 4 commits into from
Mar 29, 2024
Merged

Restructure library #2

merged 4 commits into from
Mar 29, 2024

Conversation

jhuckabee
Copy link
Contributor

Status

IN DEVELOPMENT

Description

This PR aims to initiate a discussion on optimizing the structure of the this package. The current approach of bundling all exports into a single file introduces challenges, especially when integrating with other libraries. Notably, this package exports common constants (e.g., Stream, Duration) that lead to naming conflicts.

The necessity for a revised structure is underscored by ongoing issues highlighted in the project's most recent commit. That commit reflects struggles with the current export strategy, mirroring the challenges I've encountered.

The approach presented here involves segregating generated code within the lib/src directory and creating individual library files in the lib directory. Such a structure would refine import statements, enabling more granular and conflict-free integration:

import 'package:google_cloudevents_dart/events/cloud/firestore/v1.dart';

// For isolated imports where naming conflicts may occur:
import 'package:google_cloudevents_dart/protobuf.dart' show Timestamp;

This is loosely inspired by the structure of the googleapis package.

This restructuring proposal aims not only to resolve naming conflicts but also to enhance the package's usability by allowing users to import only what they need, reducing clutter and potential conflicts.

I am eager to hear thoughts on this proposal. If this is not an ideal approach, are there alternatives that might be better suited?

Note: Instead of modifying scripts/generate_cloudevent_library.dart, I created a new file in scripts/generate_cloudevent_library_files.dart. If this proposal is something we want to move forward with, the intention is that these would get merged.

Type of Change

  • ✨ New feature (non-breaking change which adds functionality)
  • 🛠️ Bug fix (non-breaking change which fixes an issue)
  • ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 Code refactor
  • ✅ Build configuration change
  • 📝 Documentation
  • 🗑️ Chore

@jhuckabee jhuckabee changed the title Restructure library. Restructure library Mar 28, 2024
@a-wallen
Copy link
Owner

@jhuckabee I agree that this will move the package in the right direction.

This library is closely related to googleapis, so it makes sense to follow the same convention.

So, let's do it - I'm all for creating individual library files to export all the symbols without collisions even if it's a breaking change.

I think that the next steps are:

  • Merge scripts/generate_cloudevent_library.dart and scripts/generate_cloudevent_library_files.dart
  • Update the pubspec.yaml file with a new version 0.0.2.
  • Update CHANGELOG.md with release notes.

Is there anything that I can do to help move this contribution along?

@jhuckabee
Copy link
Contributor Author

@a-wallen - thanks for the feedback. I've made your suggested updates. Additionally, I've added an example application showing how to use this package in a server application (using dart_frog).

@@ -0,0 +1,19 @@
import 'package:dart_frog/dart_frog.dart';
import 'package:google_cloudevents_dart/events/cloud/firestore/v1.dart';
import 'package:http/http.dart' as http;
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if we should provide an example without HTTP as a dependency.

IIRC, the logic looks something like this

  final bytes = await context.request.bytes().fold(<int>[], (result, chunk) => [...result, ...chunk]);
  final event = DocumentEventData.fromBuffer(Uint8List.fromList(bytes));

@a-wallen a-wallen merged commit 10266ba into a-wallen:main Mar 29, 2024
@a-wallen
Copy link
Owner

LGTM - speaking of handling cloud events with dart_frog, check out this issue that I filed in the dart_frog repository @jhuckabee VeryGoodOpenSource/dart_frog#1319, I'm wondering what your thoughts are.

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.

2 participants