fix(compat): node compatibility #140

eunjae-lee · 2019-07-24T12:15:56Z

Node compatibility

1. Rollup Config

This modifies the rollup config so that it can produce two builds, one for umd and another one for cjs.

Note

I thought it's unnecessary to have codes like document.cookie in the build output for node environment, then realized there is no way to figure out the environment in the building stage because cjs build can be used both in node and browser.

So the final rollup config is just building cjs and umd with no special configuration.

2. Handling cookie

This disables anonymous tracking on env without cookies.

It's almost copy&paste from #103

3. Handling sendBeacon

This provides fallbacks to sendBeacon function.
It tries in this order:

navigator.sendBeacon
XMLHttpRequest
"http" or "https" from node

rollup.config.js

Haroenv · 2019-07-24T13:40:10Z

Just FYI, I don't think this is the best use for a stacked PR, since they all three are unrelated, and thus should be their own commit on master. You can make PRs that target master directly individually when there's no dependency on the previous PR

eunjae-lee · 2019-07-24T13:45:58Z

Just FYI, I don't think this is the best use for a stacked PR, since they all three are unrelated, and thus should be their own commit on master. You can make PRs that target master directly individually when there's no dependency on the previous PR

You're right. There's no dependencies between the PRs. I did it just for practice and thought they're okay to be merged into one commit named 'node compatibility'. If you think it should be splitted into their own commits, let me know.

package.json

rollup.config.js

eunjae-lee · 2019-07-29T13:09:36Z

making two entries: efafd97
expose aa function to node build: 0f8252e

The things like passing requester at the entry points will be done in part-2 and part-3.
what do you think? @tkrugg @samouss

lib/__tests__/_algoliaAgent.test.ts

rollup.config.js

lib/__tests__/_cookieUtils.test.ts

lib/global.d.ts

tests/utils.ts

package.json

lib/__tests__/click.test.ts

lib/_getAa.ts

Haroenv

looks mostly good

lib/__tests__/_algoliaAgent.test.ts

lib/_cookieUtils.ts

lib/_sendEvent.ts

Haroenv · 2019-08-21T11:41:12Z

lib/insights.ts

@@ -56,6 +57,10 @@ declare global {
  }
 }

+type AlgoliaAnalyticsOptions = {


this isn't reused, so can be inline, but fine like this too

lib/utils/getRequesterForBrowser.ts

lib/utils/request.ts

Co-Authored-By: Haroen Viaene <[email protected]>

…sights.js into fix/node-compat-rollup

francoischalifour · 2019-08-21T12:02:33Z

lib/__tests__/_sendEvent.node.test.ts

+    aa("init", credentials);
+  });
+
+  it("throws when user token is not set", () => {


Is userToken: '' a valid case? It's not tested but it works with this value.

We should track corner cases like this in tests.

it's already handled by the API, you get {"status":422,"message":"The userToken field is required"} so it's not a big deal if the client lets it slip.

What's the difference with an undefined userToken then?

lib/__tests__/init.test.ts

lib/_sendEvent.ts

tkrugg · 2019-08-21T13:38:49Z

lib/__tests__/_sendEvent.node.test.ts

+        userToken: "",
+        ...defaultPayload
+      });
+    }).toThrowError("`userToken` cannot be an empty string.");


To be 100% rigorous I would split this into to different it to fully reset the state between the first call and second call in the beforeEach.

Oh right. I didn't think of resetting the state between cases. ad4bea2

lib/utils/__tests__/request.test.ts

Co-Authored-By: Youcef Mammar <[email protected]>

…sights.js into fix/node-compat-rollup

lib/node.ts

tkrugg · 2019-08-21T16:36:00Z

it’d be good to later add a test for this to make sure it will never happen. But we can do it in another PR, this one is big enough.

eunjae-lee · 2019-08-21T16:39:55Z

it’d be good to later add a test for this to make sure it will never happen. But we can do it in another PR, this one is big enough.

You mean adding a test to check if the bundle output is correct, right?

* docs(readme): update documentation (#153) * docs(readme): update documentation * docs(migration): add migration file * chore(package): update description * docs(readme): fix license link (#154) * chore(deps): update dependency html-webpack-plugin to v3 (#85) * chore(deps): update dependency @types/jest to v24.0.18 (#147) * chore(deps): update dependency dotenv to v8.1.0 (#152) * chore(deps): update dependency jest to v24.9.0 (#150) * chore(deps): update node.js to v8.16.1 (#151) * chore(deps): update dependency puppeteer to v1.19.0 (#124) * docs(readme): convert `positions` to numbers (#156) * fix(compat): node compatibility (#140) * chore: configure rollup to generate two builds * chore: remove unused option (browser) from rollup config * chore: add process.maybeNode * chore: add __BROWSER_BUILD__ instead of process.maybeNode * chore: make two entries * chore: export aa function in node build * chore: instantiate AlgoliaInsights every time in tests * chore: rename entry files * chore: use top-level beforeEach * chore: rename getAa to getFuctionalInterface * fix(compat): node compatibility [PART-2] (#141) * fix(compat): disable anonymous tracking on env without cookies * chore: move supportsCookies to featureDetection * chore: process queue only on browser * chore: throw when user token is not set * chore: add jest watch typeahead plugin (#143) * chore: add jest watch typeahead plugin * chore: pin dependency * chore: add __BROWSER_BUILD__ instead of process.maybeNode * chore: add __BROWSER_BUILD__ condition * chore: fix broken test * chore: remove __BROWSER_BUILD__ * chore: process queue only on browser build * fix: process queue inside browser module * chore: fix test cases * chore: update travis config regarding xvfb (#144) * chore: rename getAa to getFuctionalInterface * chore: remove the duplicates * fix: remove useless argument * chore: move getInsatnceXX functions to _instance * fix(compat): node compatibility [PART-3] (#142) * chore: add util functions and tests * chore: add request function which wraps sendBeacon, xhr and node http * chore: apply feedback Co-Authored-By: Youcef Mammar <[email protected]> * chore: apply feedbacks Co-Authored-By: Youcef Mammar <[email protected]> * chore: require http/https dynamically to clean browser build * chore: remove node http related code from browser build * chore: remove __BROWSER_BUILD__ * chore: fix tests for sendEvent in node * fix: pass requestFn to AlgoliaAnalytics * chore: remove _instance and use AlgoliaAnalytics instead * chore: split getRequester * fix: change parameters of request for node 8.x node 8.x and 10.x have different signautures for 'request' function. - node 8.x: https://nodejs.org/docs/latest-v8.x/api/https.html#https_https_request_options_callback - node 10.x: https://nodejs.org/dist/latest-v10.x/docs/api/https.html#https_https_request_options_callback * fix: allow userToken in event payload * fix: remove the duplicated assignment * fix: check userToken for only undefined * chore: rename AlgoliaInsights with analyticsInstance in test cases * fix: add `new` at `TypeError`s Co-Authored-By: Haroen Viaene <[email protected]> * chore: apply suggestions from code review Co-Authored-By: Haroen Viaene <[email protected]> * fix: update inline snapshot of test case * fix: add `new` at `TypeError`s * chore: inline AlgoliaAnalyticsOptions type * chore: use `isUndefined` helper * fix: throw when userToken is an empty string * fix: improve test case * fix: split a test case * chore: update feedback from review Co-Authored-By: Youcef Mammar <[email protected]> * docs: add a section for Node.js module (#145) * docs: add a section for node module * docs: add TOC * docs: apply feedbacks Co-Authored-By: Youcef Mammar <[email protected]> * docs: move Node.js part under getting started * docs: update setUserToken part * docs: apply changes again * docs: apply suggestions from code review Co-Authored-By: François Chalifour <[email protected]> * docs: apply feedbacks * docs: update TOC * chore: update version

chore: configure rollup to generate two builds

41811f6

eunjae-lee added the ⚠️ DO NOT MERGE label Jul 24, 2019

eunjae-lee mentioned this pull request Jul 24, 2019

fix(compat): node compatibility [PART-2] #141

Merged

Haroenv reviewed Jul 24, 2019

View reviewed changes

rollup.config.js Outdated Show resolved Hide resolved

eunjae-lee mentioned this pull request Jul 25, 2019

fix(compat): node compatibility [PART-3] #142

Merged

eunjae-lee requested a review from a team July 25, 2019 14:46

ghost requested review from Haroenv and tkrugg and removed request for a team July 25, 2019 14:46

tkrugg reviewed Jul 26, 2019

View reviewed changes

package.json Show resolved Hide resolved

Eunjae Lee added 4 commits July 26, 2019 16:59

chore: remove unused option (browser) from rollup config

bc1ff80

chore: add process.maybeNode

cfcbed9

chore: add __BROWSER_BUILD__ instead of process.maybeNode

45fb4f8

Merge branch 'develop' into fix/node-compat-rollup

6c2af9a

samouss reviewed Jul 29, 2019

View reviewed changes

rollup.config.js Show resolved Hide resolved

Eunjae Lee added 2 commits July 29, 2019 14:53

chore: make two entries

efafd97

chore: export aa function in node build

0f8252e

samouss reviewed Jul 30, 2019

View reviewed changes

lib/__tests__/_algoliaAgent.test.ts Outdated Show resolved Hide resolved

rollup.config.js Outdated Show resolved Hide resolved

Eunjae Lee added 2 commits July 30, 2019 13:27

chore: instantiate AlgoliaInsights every time in tests

95ec9aa

chore: rename entry files

db1533e

eunjae-lee commented Jul 30, 2019

View reviewed changes

lib/__tests__/_cookieUtils.test.ts Show resolved Hide resolved

Merge branch 'develop' into fix/node-compat-rollup

a447236

samouss reviewed Jul 31, 2019

View reviewed changes

lib/global.d.ts Outdated Show resolved Hide resolved

tests/utils.ts Outdated Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

lib/__tests__/click.test.ts Outdated Show resolved Hide resolved

lib/_getAa.ts Outdated Show resolved Hide resolved

Eunjae Lee added 2 commits July 31, 2019 15:53

chore: use top-level beforeEach

7f4217b

chore: rename getAa to getFuctionalInterface

f231802

eunjae-lee force-pushed the fix/node-compat-rollup branch from ade891e to f231802 Compare July 31, 2019 15:24

Haroenv approved these changes Aug 5, 2019

View reviewed changes

eunjae-lee requested review from Haroenv and samouss August 21, 2019 11:35

Haroenv reviewed Aug 21, 2019

View reviewed changes

Eunjae Lee and others added 7 commits August 21, 2019 13:52

chore: rename AlgoliaInsights with analyticsInstance in test cases

7a151b4

fix: add new at TypeErrors

781377d

Co-Authored-By: Haroen Viaene <[email protected]>

chore: apply suggestions from code review

cfe2227

Co-Authored-By: Haroen Viaene <[email protected]>

Merge branch 'fix/node-compat-rollup' of github.com:algolia/search-in…

a4a3495

…sights.js into fix/node-compat-rollup

fix: update inline snapshot of test case

f4d20d4

fix: add new at TypeErrors

c5eb30e

chore: inline AlgoliaAnalyticsOptions type

5d972fb

francoischalifour reviewed Aug 21, 2019

View reviewed changes

tkrugg reviewed Aug 21, 2019

View reviewed changes

lib/_sendEvent.ts Outdated Show resolved Hide resolved

Eunjae Lee added 3 commits August 21, 2019 14:31

chore: use isUndefined helper

c6eddf0

fix: throw when userToken is an empty string

9f8559d

fix: improve test case

4803db6

tkrugg reviewed Aug 21, 2019

View reviewed changes

lib/utils/__tests__/request.test.ts Outdated Show resolved Hide resolved

tkrugg approved these changes Aug 21, 2019

View reviewed changes

Eunjae Lee and others added 3 commits August 21, 2019 15:51

fix: split a test case

ad4bea2

chore: update feedback from review

019d91f

Co-Authored-By: Youcef Mammar <[email protected]>

Merge branch 'fix/node-compat-rollup' of github.com:algolia/search-in…

9f8b821

…sights.js into fix/node-compat-rollup

tkrugg reviewed Aug 21, 2019

View reviewed changes

lib/node.ts Show resolved Hide resolved

Merge branch 'develop' into fix/node-compat-rollup

22f3557

tkrugg merged commit e804bd1 into develop Aug 26, 2019

tkrugg deleted the fix/node-compat-rollup branch August 26, 2019 16:00

tkrugg removed the ⚠️ DO NOT MERGE label Nov 15, 2019

tkrugg mentioned this pull request Jan 15, 2020

Specify env in user agent #157

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(compat): node compatibility #140

fix(compat): node compatibility #140

eunjae-lee commented Jul 24, 2019 •

edited

Loading

Haroenv commented Jul 24, 2019

eunjae-lee commented Jul 24, 2019

eunjae-lee commented Jul 29, 2019

Haroenv left a comment

Haroenv Aug 21, 2019

eunjae-lee Aug 21, 2019

francoischalifour Aug 21, 2019

tkrugg Aug 21, 2019 •

edited

Loading

francoischalifour Aug 21, 2019

eunjae-lee Aug 21, 2019

tkrugg Aug 21, 2019

eunjae-lee Aug 21, 2019

tkrugg commented Aug 21, 2019 via email •

edited

Loading

eunjae-lee commented Aug 21, 2019

fix(compat): node compatibility #140

fix(compat): node compatibility #140

Conversation

eunjae-lee commented Jul 24, 2019 • edited Loading

Node compatibility

1. Rollup Config

Note

2. Handling cookie

3. Handling sendBeacon

Haroenv commented Jul 24, 2019

eunjae-lee commented Jul 24, 2019

eunjae-lee commented Jul 29, 2019

Haroenv left a comment

Choose a reason for hiding this comment

Haroenv Aug 21, 2019

Choose a reason for hiding this comment

eunjae-lee Aug 21, 2019

Choose a reason for hiding this comment

francoischalifour Aug 21, 2019

Choose a reason for hiding this comment

tkrugg Aug 21, 2019 • edited Loading

Choose a reason for hiding this comment

francoischalifour Aug 21, 2019

Choose a reason for hiding this comment

eunjae-lee Aug 21, 2019

Choose a reason for hiding this comment

tkrugg Aug 21, 2019

Choose a reason for hiding this comment

eunjae-lee Aug 21, 2019

Choose a reason for hiding this comment

tkrugg commented Aug 21, 2019 via email • edited Loading

eunjae-lee commented Aug 21, 2019

eunjae-lee commented Jul 24, 2019 •

edited

Loading

tkrugg Aug 21, 2019 •

edited

Loading

tkrugg commented Aug 21, 2019 via email •

edited

Loading