Skip to content

Conversation

adams85
Copy link
Collaborator

@adams85 adams85 commented Aug 27, 2024

Describe the purpose of your pull request

The initial state of this repo is a copy of https://github.com/configcat/common-js. This PR merges the following repos containing platform-specific JS SDKs into that code base:

The goal is to end up with a single NPM package that can be used as a drop-in replacement for the listed packages.

This will allow us to eliminate a lot of redundancy (especially w.r.t. build configuration and documentation), improve development velocity and, thus, significantly reduce maintenance costs. As a side effect, the unified package will also provide a better support for SSR applications.

Additional benefits and improvements:

  • We can drop the axios dependency as it is not needed anymore. The platform-specific targeting is already handled via the main / browser and exports package.json fields.
  • The tests specified in common-js have only been run on Node.js so far. Now we can run them on all the target platforms, which increases reliability.
  • Tests have used the source code (src) so far instead of the compiled code (lib/dist). The build configuration have been adjusted so now we test the code we actually distribute.
  • New dist bundles have been added, so consumers who don't want to support ancient browsers can use more lightweight builds of the library.
  • The intermediate lib builds for Node.js and bundlers now targets ES2017, which allows consumers to choose the degree of downleveling, which can save many kilobytes if support for old browsers is not necessary.
  • This also allows us to drop the tslib dependency, which in turn can also reduce application bundle sizes.
  • A few nasty non-deterministic (GC-related) bugs have been fixed in the tests.
  • Test running in the browser has been revised and cleaned up, which made it possible to get rid of some problematic dependencies. Now the code coverage collection and aggregation is entirely based on istanbuljs.
  • Sample apps have been updated and a bunch of new sample apps have been added. Thanks to this, the package have also been tested with the widely used bundlers.
  • Support for Deno.
  • Minor performance improvements (execution and bundle size).

Requirement checklist (only if applicable)

  • I have covered the applied changes with automated tests.
  • I have executed the full automated test set against my changes.
  • I have validated my changes against all supported platform versions.
  • I have read and accepted the contribution agreement.

@adams85 adams85 force-pushed the package-consolidation branch 2 times, most recently from aec23b7 to e30219f Compare August 28, 2024 14:46
@adams85 adams85 force-pushed the package-consolidation branch from e30219f to 07e52d6 Compare August 28, 2024 14:58
@adams85 adams85 force-pushed the package-consolidation branch from c321755 to 40f7279 Compare August 29, 2024 14:31
@adams85 adams85 force-pushed the package-consolidation branch from 40f7279 to 9c34929 Compare August 29, 2024 15:11
@adams85 adams85 force-pushed the package-consolidation branch 8 times, most recently from e271055 to 754307e Compare August 30, 2024 17:34
@adams85 adams85 force-pushed the package-consolidation branch 2 times, most recently from fd27332 to 5318552 Compare October 12, 2024 14:01
@adams85 adams85 force-pushed the package-consolidation branch from 5318552 to 801be47 Compare October 12, 2024 14:03
Copy link

Copy link
Member

@z4kn4fein z4kn4fein left a comment

Choose a reason for hiding this comment

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

Just 2 small suggestions, but overall, it looks good to me!

README.md Outdated
@@ -1,38 +1,244 @@
# ConfigCat Common library for JavaScript
# ConfigCat SDK for JavaScript
https://configcat.com
Copy link
Member

Choose a reason for hiding this comment

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

I'd remove this link from the top of the README (it'll be in the About section of the repo anyway)

README.md Outdated
ConfigCat SDK for JavaScript provides easy integration for your application to ConfigCat.

ConfigCat Common library for JavaScript is a shared package that provides the common ConfigCat SDK logic for [ConfigCat SDK for Node.js](https://github.com/configcat/node-sdk) and [ConfigCat SDK for JavaScript](https://github.com/configcat/js-sdk).
## About
Copy link
Member

@z4kn4fein z4kn4fein Nov 6, 2024

Choose a reason for hiding this comment

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

I'd move this to the end of the readme under an About ConfigCat section. IMO this part of the README should be about the repo's content.

Copy link

sonarqubecloud bot commented Nov 8, 2024

@adams85 adams85 merged commit 8906302 into master Nov 8, 2024
16 of 36 checks passed
@z4kn4fein z4kn4fein deleted the package-consolidation branch November 25, 2024 08:58
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