launchdarkly / react-client-sdk

LaunchDarkly Client-side SDK for React.js
Other
81 stars 67 forks source link

allow to export LDClient, LDFlagSet, LD Options, LDUser interfaces from 'launchdarkly-js-client-sdk' #100

Closed glebpigulevsky closed 2 years ago

glebpigulevsky commented 2 years ago

Is your feature request related to a problem? Please describe. I have to install additionally 'launchdarkly-js-client-sdk' to import these interfaces.

Describe the solution you'd like add the necessary interfaces to index.ts:

import withLDProvider from './withLDProvider';
import asyncWithLDProvider from './asyncWithLDProvider';
import withLDConsumer from './withLDConsumer';
import useFlags from './useFlags';
import useLDClient from './useLDClient';
import { camelCaseKeys } from './utils';
import { LDClient, LDFlagSet, LDOptions, LDUser } from 'launchdarkly-js-client-sdk';

export {
  LDProvider,
  withLDProvider,
  withLDConsumer,
  useFlags,
  useLDClient,
  asyncWithLDProvider,
  camelCaseKeys,
  LDClient,
  LDFlagSet,
  LDOptions,
  LDUser,
};
eli-darkly commented 2 years ago

Are you sure you have to install launchdarkly-js-client-sdk? I don't see any reason you wouldn't be able to write this same import in your own code, without changing anything in your package.json:

import { LDClient, LDFlagSet, LDOptions, LDUser } from 'launchdarkly-js-client-sdk';

It sounds like maybe the problem you're having is just that you are trying to do this:

import { LDClient, LDFlagSet, LDOptions, LDUser } from 'launchdarkly-react-client-sdk';
glebpigulevsky commented 2 years ago

you're right. I was confused when moved my project from js-client to react-client sdk. and didn't remove launchdarkly-js-client-sdk

kbirger commented 2 years ago

I can confirm this works, but it is generally not a good idea to import dependencies that are not explicitly specified in the dependencies block of your own package.

For example, there's a lint rule which exists for this case: https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/no-extraneous-dependencies.md

Unfortunately the documentation does not express the rationale behind the rule. However, the general thinking is this: If I import a package that's in your dependencies, I don't see when it changes. If someone working on my project happens to use it somewhere else, it might change without any explicit action on our project, and it is not very transparent without looking through node_modules what version is being used.

So, while the solution to

import { LDClient, LDFlagSet, LDOptions, LDUser } from 'launchdarkly-js-client-sdk';

without referencing it works, it would still be much better to have an official type exported from react-client-sdk.

It could be the same type as in launchdarkly-js-client-sdk, or you could define your own type. Defining your own type would allow you, in a future version, to extend the type to allow you to provide additional functionality in your package. It also would complete the encapsulation in your abstraction over the underlying package.

Just a thought :)

eli-darkly commented 2 years ago

However, the general thinking is this: If I import a package that's in your dependencies, I don't see when it changes. If someone working on my project happens to use it somewhere else, it might change without any explicit action on our project, and it is not very transparent without looking through node_modules what version is being used.

I understand that general issue with transitive dependencies, but I'm not sure it is applicable in this case.

Every version of launchdarkly-react-client-sdk uses one specific version of launchdarkly-js-client-sdk. That dependency only changes deliberately and it will always be reflected in the changelog for a React SDK release, and our goal is to always put out a new React SDK release whenever we have fixed or enhanced the JS SDK. The changelog description will either be like "updated JS SDK to version ____" (with a link to the JS SDK release so you can see exactly what is new), or it will mention the same things that were mentioned in the JS SDK changelog. There is no ambiguity there and you should never be uncertain of whether something has changed.

Defining your own type would allow you, in a future version, to extend the type to allow you to provide additional functionality in your package. It also would complete the encapsulation in your abstraction over the underlying package.

We've deliberately avoided using that strategy. The React SDK is meant to be a thin wrapper over the JS SDK, and we prefer to keep it that way and make that explicit in how it is documented and how the types are defined, rather than trying to hide that relationship and make it look like entirely its own thing.

The vast majority of JS SDK functionality is totally independent of whether you're using React or not, and we don't want to have to mirror every type from that package into this one just to make that functionality visible. And it really would have to be every type— the proposed solution of re-exporting just the four particular types mentioned in the issue title would not work, since some of those types and methods reference other types like LDEvaluationDetail and LDLogger, so if you wanted to be able to use those without ever referencing launchdarkly-js-client-sdk we would have to re-export everything, and keep those exports up to date whenever something new was added.

bwoskow-ld commented 2 years ago

Hi @glebpigulevsky,

We decided against making a code change to resolve this. However, we did improve our SDK documentation to clarify how to handle this situation.