Skip to content

Publish shared components #616

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

Closed
toddbaert opened this issue Oct 26, 2023 · 7 comments
Closed

Publish shared components #616

toddbaert opened this issue Oct 26, 2023 · 7 comments
Assignees

Comments

@toddbaert
Copy link
Member

toddbaert commented Oct 26, 2023

Currently we do not publish a "shared" module for common type/interfaces/classes between the web and server SDKs. This hasn't been too much of an issue so far, particularly since providers don't share exactly the same interface between both SDKs. However, recently, there's been a demonstrable need for a common artifact, for example, the flagd-in-process lib here. This artifact could be useful in both web/server contexts. There's probably more use-cases as well (maybe even some hooks?).

Now that we know there's a use case, we should:

  • publish the shared module (we can decide on a good name)
  • set it as a dependency of the web/server/react SDKs
  • re-export all dependencies from it in the server/web SDKs (this will prevent us from requiring users to update all their imports)
@toddbaert
Copy link
Member Author

@thomaspoignant @lukas-reining you guys have any concerns with this?

@lukas-reining
Copy link
Member

Well, sounds like a good idea to me.
My only concern is that we could not have any shared "SDK internals" then. If we put all the stuff the SDKs share in this package, anyone using it may depend on implementation details that are not meant to be part of the public api.
We could avoid this by having the published shared package and shared internals separated but I fear this may lead to inconsistencies🤔
Or maybe this is just not a problem because we do not have such "implementation details" that we would expose, I have no time to check right now but will have a look tomorrow.

@toddbaert
Copy link
Member Author

Well, sounds like a good idea to me. My only concern is that we could not have any shared "SDK internals" then. If we put all the stuff the SDKs share in this package, anyone using it may depend on implementation details that are not meant to be part of the public api. We could avoid this by having the published shared package and shared internals separated but I fear this may lead to inconsistencies🤔 Or maybe this is just not a problem because we do not have such "implementation details" that we would expose, I have no time to check right now but will have a look tomorrow.

Currently, we bundle and publish the contents of "shared" already, as part of the web/server modules - if they are exported, they are already exposed publicly through those modules.

I can't think of anything that fits into this category though, besides maybe the OpenFeatureCommonAPI abstract class... and I'm not sure it's really much of a problem. 🤔

@thomaspoignant
Copy link
Member

I like the idea a lot, and it can avoid mixing things in a provider.
It happened to me for example with the EvaluationContext see open-feature/js-sdk-contrib#583.

Using a shared component may help to avoid that.

@lukas-reining
Copy link
Member

lukas-reining commented Oct 31, 2023

if they are exported, they are already exposed publicly through those modules

Yes, but currently we can decide what to export instead of completely exposing everything directly used by one of the SDKs.

... and I'm not sure it's really much of a problem.

Ya, thats what I meant. I currently also see nothing in the package that could lead to a problem if someone depends on it.
So I think even though something like this might come up later, it should not be problematic as I think no one would use such a thing then anyways :)

So I think this change really helps and the potential issue is negligible.

@toddbaert
Copy link
Member Author

toddbaert commented Oct 31, 2023

So I think even though something like this might come up later,

I was thinking about this more, and it's a good thing to be aware of. I think that we could actually have 2 exports from the package, one at the root, and one at /internal, which we could even hide from the published version.

I think it is a good idea to hide as much that isn't API surface as possible... so we should consider this if we add things in that category (agree there isn't much like this right now).

github-merge-queue bot pushed a commit that referenced this issue Oct 31, 2023
This PR addresses: #616 by:

- configuring CI to release a new module (in addition to
server/web/react we now release the "core" module (previously called
"shared").
- adding a peer dep to the above module to server/web/react
- adding a workflow to ensure that `shared` is released ahead of
associated web/server/react changes, and which also auto-increments the
peer dep version when the core module is released
- unrelated `typedoc` improvements/updates

> [!NOTE]  
> I intend to create a new PR that will rename the dir structure
according to the packages... so `shared/` will become `core/` and
`client/` will become `web/`. I just didn't want to add confusion and
noise to this PR.

---------

Signed-off-by: Todd Baert <[email protected]>
@toddbaert
Copy link
Member Author

This is done, and working. We now also have validation that makes sure shared is released first and all dependent modules have a peer dep for the latest version on release.

github-merge-queue bot pushed a commit that referenced this issue Nov 3, 2023
Optimization to `@openfeature/core` package. Both server and web need
the node-js `Events` interfaces, but obviously only the web needs them
poly-filled (they come with node).

This PR:

- marks `events.js` as an external dep in `@openfeature/core` meaning it
won't be bundled there (see esbuild
[doc](https://esbuild.github.io/api/#external))

This means that now `@openfeature/core` no longer includes `events.js`
(an un-needed polyfill for the server). The bundling pictured below only
happens in the `dist/` of `@openfeature/web-sdk`, and no longer in
`@openfeature/core` :


![image](https://github.com/open-feature/js-sdk/assets/25272906/2cfdce09-7f00-42a7-b797-9570c816cadb)

I've tested both the server and web SDKs based on this change locally in
the js-contribs by running the e2e test suite.

Relates to: #616

Signed-off-by: Todd Baert <[email protected]>
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

No branches or pull requests

3 participants