feat: Added a Lite bundle excluding Data file manager and Event Processor packages. #699

zashraf1985 · 2021-08-24T21:43:15Z

Summary

Some platforms (such as edge providers), do not need extended functionality offered by data file manager and event processor. This PR creates a lite bundle which uses a NoOpDatafileManager and a ForwardingEventProcessor which immediately dispatches events when a custom event dispatcher is provided. This results in up to 20% reduction in bundle size. This also fixes the errors that SDK was throwing on edge platforms.

Usage

To use the lite bundle, user needs to explicitly import from it.
import { createInstance } from '@optimizely/optimizely-sdk/dist/optimizely.lite.es.mins.js'

Test plan

All the Full stack compatibility suite tests pass for the existing bundles.
Fixed all the failing unit tests.
Added new unit tests to lite entry point.

coveralls · 2021-08-24T21:55:26Z

Coverage increased (+0.08%) to 97.072% when pulling f94e45b on yasir/edge-support-testing into 7a73b91 on master.

…nctionality to forwarding event processor.

yavorona · 2021-09-21T17:30:15Z

packages/optimizely-sdk/lib/core/event_builder/build_event_v1.ts

@@ -0,0 +1,257 @@
+import {


Let's add a License header

what does event_v1 means?

v1 is the version of event on logx end point

yavorona · 2021-09-21T17:31:56Z

packages/optimizely-sdk/lib/core/event_builder/build_event_v1.ts

+  tags?: EventTags
+}
+
+type Attributes = {


I don't see this type being used anywhere, so we should delete it

We already have an Attribute type called UserAttributes in shared_types, can't we use that?

yavorona · 2021-09-21T17:41:35Z

packages/optimizely-sdk/lib/plugins/event_processor/forwarding_event_processor.tests.js

+    ep.stop().then(done);
+  });  
+ });
+


NIT: remove blank line

yavorona · 2021-09-21T20:07:02Z

packages/optimizely-sdk/lib/index.lite.ts

+  createInstance,
+  OptimizelyDecideOption,
+};
+


NIT: extra blank line

The-inside-man · 2021-09-21T20:23:25Z

packages/optimizely-sdk/lib/core/event_builder/build_event_v1.ts

+ * @export
+ * @interface EventBuilderV1
+ */
+


NIT: Extra blank line here

yavorona · 2021-09-22T00:05:41Z

packages/optimizely-sdk/lib/plugins/datafile_manager/no_op_datafile_manager.ts

@@ -0,0 +1,27 @@
+import { DatafileManager, DatafileUpdateListener} from '../../shared_types';


It doesn't seem like we use these types anywhere else outside of this module, so we can define them here. The intension for shared_types to only contain types that are shared across modules.

These are generic interfaces and are being used in the main optimizely class in its constructor

yavorona

LGTM except for a couple of minor comments. I also tested it locally and it works as expected!

msohailhussain

have some questions.

msohailhussain · 2021-09-23T20:38:46Z

packages/optimizely-sdk/lib/core/event_builder/build_event_v1.ts

@@ -0,0 +1,257 @@
+import {


what does event_v1 means?

msohailhussain · 2021-09-23T20:41:53Z

packages/optimizely-sdk/lib/core/event_builder/build_event_v1.ts

+  tags?: EventTags
+}
+
+type Attributes = {


We already have an Attribute type called UserAttributes in shared_types, can't we use that?

msohailhussain · 2021-09-23T20:43:37Z

packages/optimizely-sdk/lib/core/event_builder/build_event_v1.ts

+  }
+}
+
+function makeConversionSnapshot(conversion: ConversionEvent): Snapshot {


assuming this whole code is copied from buildEventV1

yes! copied from event processor because we wanted to create a forwarding event processor without importing that package and we still needed this to build the payload.. I know this is not the best way to do it but we plan to consolidate those packages into one anyway. this redundancy will be solved then.

msohailhussain · 2021-09-23T20:44:57Z

packages/optimizely-sdk/lib/core/project_config/project_config_manager.ts

  OnReadyResult,
-  OptimizelyConfig
+  OptimizelyConfig,
+  DatafileManager,


so you are preparing ConfigManager outside and importing here?

Building datafileManager outside and including here. The DatafileManager in this particular import is the interface.

msohailhussain · 2021-09-23T21:45:23Z

packages/optimizely-sdk/lib/index.node.ts

+    const errorHandler = getErrorHandler();
+    const notificationCenter = createNotificationCenter({ logger: logger, errorHandler: errorHandler });
+
+    const eventProcessorConfig = {


I see a lot of common code in index.browser / react / node . ts file, It would be good if we can have a separate util class from where we create eventProcess/ Config & other stuff which is common.

Yes! you are right in principle. There is some common code but there are some subtle differences too. I would recommend to leave it like this for this PR and take it up separately later. what do you say?

msohailhussain

lgtm

The-inside-man · 2021-10-04T17:30:48Z

packages/optimizely-sdk/lib/core/project_config/project_config_manager.tests.js

@@ -424,6 +436,7 @@ describe('lib/core/project_config/project_config_manager', function() {
        var manager = projectConfigManager.createProjectConfigManager({
          datafile: testData.getTestProjectConfig(),
          sdkKey: '12345',
+          datafileManager: createHttpPollingDatafileManager('12345', logger, testData.getTestProjectConfig()),


Not sure if necessary but would it make sense to pull out the test value of '12345' and move to a constant?

The-inside-man · 2021-10-04T17:33:59Z

packages/optimizely-sdk/lib/plugins/datafile_manager/http_polling_datafile_manager.tests.js

+      sandbox.stub(projectConfig, 'tryCreatingProjectConfig').returns({ configObj: null, error: 'Error creating config'});
+      sandbox.stub(projectConfig, 'toDatafile');
+    });
+    it('Should log error when error is thrown while creating project config', () => {


NIT: used a capital here but not on other it( statements.

The-inside-man

Couple very small things but LGTM!

optimizely-codereview-bot assigned zashraf1985 Aug 24, 2021

yasirfolio3 and others added 12 commits September 2, 2021 07:02

Removed datafile-manager for edge. Still need to remove from unit tests.

c49f682

Improvements and fixes for unit tests.

d634269

fixes.

8cf008e

Added ESM bundle for edge

a68a918

experiment taking out event processor for edge package.

5523f7a

fixed FSC tests

77f7a6b

reorganized event dispatcher and datafile manager code. Also added fu…

1f61a04

…nctionality to forwarding event processor.

fixed lint

69b527d

Fixed project config manager tests

d78a146

Fixed optimizely tests

1587eee

All unit tests fixed

5ed7e07

fixed umd and cross browser tests

59855e6

zashraf1985 force-pushed the yasir/edge-support-testing branch from 86950ba to 59855e6 Compare September 2, 2021 14:04

zashraf1985 added 6 commits September 3, 2021 10:48

renamed bundle to lite

8e3db43

added unit tests for lite bundle

c64005e

Added test coverage for httppollingddatafilemanager wrapper

59e9ae9

Added test coverage for no op datafile manager

0e1dbb6

Added unit test coverage for missing utility function

b5c6283

Added more unit tests

753aafe

zashraf1985 changed the title ~~Feat: Experimenting Edge Bundle~~ Feat: Added a Lite bundle which does not include Data file manager and Event Processor. Sep 13, 2021

zashraf1985 removed their assignment Sep 13, 2021

zashraf1985 marked this pull request as ready for review September 13, 2021 21:49

zashraf1985 requested a review from a team as a code owner September 13, 2021 21:49

optimizely-codereview-bot added 🤖 4 hrs 🤖 8 hrs 🤖 12 hrs and removed 🤖 4 hrs 🤖 8 hrs labels Sep 14, 2021

optimizely-codereview-bot assigned zashraf1985 Sep 15, 2021

zashraf1985 added 3 commits September 19, 2021 22:19

incorporated review feedback

5b255a5

Merge branch 'master' into yasir/edge-support-testing

f6ed35a

updated unit test

b8508cc

yavorona self-requested a review September 21, 2021 17:28

yavorona reviewed Sep 21, 2021

View reviewed changes

packages/optimizely-sdk/lib/index.lite.ts Outdated

createInstance,

OptimizelyDecideOption,

};

Copy link

Contributor

yavorona Sep 21, 2021

Choose a reason for hiding this comment

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

NIT: extra blank line

The-inside-man reviewed Sep 21, 2021

View reviewed changes

yavorona reviewed Sep 22, 2021

View reviewed changes

yavorona self-requested a review September 22, 2021 17:03

yavorona approved these changes Sep 22, 2021

View reviewed changes

msohailhussain suggested changes Sep 23, 2021

View reviewed changes

incorporated more review feedback

a9c8865

zashraf1985 removed their assignment Oct 4, 2021

optimizely-codereview-bot requested review from msohailhussain and The-inside-man October 4, 2021 06:13

zashraf1985 changed the title ~~Feat: Added a Lite bundle which does not include Data file manager and Event Processor.~~ Feat: Added a Lite bundle excluding Data file manager and Event Processor packages. Oct 4, 2021

zashraf1985 changed the title ~~Feat: Added a Lite bundle excluding Data file manager and Event Processor packages.~~ feat: Added a Lite bundle excluding Data file manager and Event Processor packages. Oct 4, 2021

zashraf1985 added 3 commits October 3, 2021 23:14

Added missing copyright header

60ca121

reverted unneeded formatting changes

b9e8bd5

a little cleanup

fe7bcf7

msohailhussain approved these changes Oct 4, 2021

View reviewed changes

The-inside-man reviewed Oct 4, 2021

View reviewed changes

The-inside-man approved these changes Oct 4, 2021

View reviewed changes

Merge branch 'master' into yasir/edge-support-testing

f94e45b

zashraf1985 merged commit b0478ec into master Oct 7, 2021

zashraf1985 deleted the yasir/edge-support-testing branch October 7, 2021 21:35

		@@ -0,0 +1,27 @@
		import { DatafileManager, DatafileUpdateListener} from '../../shared_types';

feat: Added a Lite bundle excluding Data file manager and Event Processor packages. #699

feat: Added a Lite bundle excluding Data file manager and Event Processor packages. #699

Uh oh!

Conversation

zashraf1985 commented Aug 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Usage

Test plan

Uh oh!

coveralls commented Aug 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yavorona left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msohailhussain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msohailhussain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

The-inside-man left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zashraf1985 commented Aug 24, 2021 •

edited

Loading

coveralls commented Aug 24, 2021 •

edited

Loading

yavorona left a comment •

edited

Loading