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

[Player] Prepare module for event producer #91

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

enjikaka
Copy link
Member

  • Export EventSender namespace as type/interface in the common package
  • Add a setEventSender handler to player module

Modelled after setCredentialsProvider for consistency.

@enjikaka enjikaka self-assigned this Mar 21, 2024
@enjikaka enjikaka requested a review from a team as a code owner March 21, 2024 13:23
@@ -9,7 +9,13 @@ export default defineConfig({
formats: ['es'],
},
},
plugins: [dts({ rollupTypes: true, tsconfigPath: 'tsconfig.build.json' })],
Copy link
Member Author

Choose a reason for hiding this comment

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

Had to remove rollupTypes due to:

Skärmavbild 2024-03-21 kl  14 24 48

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -0,0 +1,3 @@
import type * as _EventSender from '@tidal-music/event-producer';
Copy link
Contributor

Choose a reason for hiding this comment

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

Common should not be allowed to import from any SDK module I think, or else we create circular dependencies?

Copy link
Member Author

@enjikaka enjikaka Apr 4, 2024

Choose a reason for hiding this comment

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

For type imports it's fine!

@enjikaka enjikaka changed the title Prepare player module for event producer [Player] Prepare module for event producer Apr 11, 2024
@osmestad
Copy link
Contributor

osmestad commented May 7, 2024

I have a couple of more questions :-)

  1. Why do we need to move this type to Common in the first place? if Player is only using it as a devDependency in the other PR it should not matter if the type is imported from EventProducer directly?
  2. If we want this in Common I'm not sure we want to disable rollupTypes, I think we had problems before when we needed to use that in some packages? It does not seem to me like the added bundledPackages (https://github.com/tidal-music/tidal-sdk-web/pull/91/files#diff-18384a37e9745351ae8f00189097846fbd0c20a0e95d704978e6058f8bb2157dR15) is doing anything? so maybe better to remove that and keep rollupTypes?

@andymartinwork andymartinwork self-requested a review May 7, 2024 13:21
@enjikaka
Copy link
Member Author

enjikaka commented May 7, 2024

  1. Why do we need to move this type to Common in the first place? if Player is only using it as a devDependency in the other PR it should not matter if the type is imported from EventProducer directly?

The idea was to have the same flow as for the CredentialsProvider. I'll change so the player uses it directly.

  1. If we want this in Common I'm not sure we want to disable rollupTypes, I think we had problems before when we needed to use that in some packages? It does not seem to me like the added bundledPackages (https://github.com/tidal-music/tidal-sdk-web/pull/91/files#diff-18384a37e9745351ae8f00189097846fbd0c20a0e95d704978e6058f8bb2157dR15) is doing anything? so maybe better to remove that and keep rollupTypes?

w/o rollupTypes it was needed not not include the entire EP in Common if I remember correctly. Pushed a commit in the other PR to use EP directly -> 95064cf

@andymartinwork
Copy link
Contributor

When you get back, we should try and get this out.

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.

3 participants