amplitude / Amplitude-TypeScript

TypeScript Amplitude Analytics SDK
https://amplitude.github.io/Amplitude-TypeScript/
MIT License
142 stars 40 forks source link

maj tslib to next major #206

Closed kopax-polyconseil closed 2 years ago

kopax-polyconseil commented 2 years ago

Expected Behavior

We expect not to have any duplicate module after our application build

Current Behavior

We have tslib twice, because of amplitude-js and underlying amplitude utils

## static/js/2.f752eb41.chunk.js
tslib (Found 2 resolved, 2 installed, 13 depended. Latest 2.3.1.)
  1.14.1
    ~/@amplitude/utils/~/tslib
      * Dependency graph
        PassCulture@1.200.0 -> amplitude-js@^8.16.1 -> @amplitude/utils@^1.0.5 -> tslib@^1.9.3
      * Duplicated files in static/js/2.f752eb41.chunk.js
        tslib/tslib.es6.js (S, 10845)

  2.3.1
    ~/tslib
      * Dependency graph
        PassCulture@1.200.0 -> @fingerprintjs/fingerprintjs@^3.3.0 -> tslib@^2.0.1
        PassCulture@1.200.0 -> firebase@^9.6.11 -> @firebase/analytics-compat@0.1.9 -> tslib@^2.1.0
        PassCulture@1.200.0 -> firebase@^9.6.11 -> @firebase/analytics@0.7.8 -> tslib@^2.1.0
        PassCulture@1.200.0 -> firebase@^9.6.11 -> @firebase/app-compat@0.1.22 -> tslib@^2.1.0
        PassCulture@1.200.0 -> firebase@^9.6.11 -> @firebase/app@0.7.21 -> tslib@^2.1.0
        PassCulture@1.200.0 -> firebase@^9.6.11 -> @firebase/app@0.7.21 -> @firebase/component@0.5.13 -> tslib@^2.1.0
        PassCulture@1.200.0 -> firebase@^9.6.11 -> @firebase/app@0.7.21 -> @firebase/logger@0.3.2 -> tslib@^2.1.0
        PassCulture@1.200.0 -> firebase@^9.6.11 -> @firebase/app@0.7.21 -> @firebase/util@1.5.2 -> tslib@^2.1.0
        PassCulture@1.200.0 -> firebase@^9.6.11 -> @firebase/firestore-compat@0.1.17 -> tslib@^2.1.0
        PassCulture@1.200.0 -> firebase@^9.6.11 -> @firebase/firestore@3.4.8 -> tslib@^2.1.0
        PassCulture@1.200.0 -> firebase@^9.6.11 -> @firebase/installations@0.5.8 -> tslib@^2.1.0
        PassCulture@1.200.0 -> firebase@^9.6.11 -> @firebase/remote-config@0.3.7 -> tslib@^2.1.0
      * Duplicated files in static/js/2.f752eb41.chunk.js
        tslib/tslib.es6.js (S, 12348)

* Understanding the report: Need help with the details? See:
  https://github.com/FormidableLabs/inspectpack/#diagnosing-duplicates
* Fixing bundle duplicates: An introductory guide:
  https://github.com/FormidableLabs/inspectpack/#fixing-bundle-duplicates

Possible Solution

Upgrade tslib in utils to ^2.0.0

Such as what as been done in https://github.com/amplitude/Amplitude-Node/issues/176

Steps to Reproduce

  1. git clone https://github.com/pass-culture/pass-culture-app-native.git
  2. cd pass-culture-app-native
  3. remove this line in package.json : https://github.com/pass-culture/pass-culture-app-native/blob/master/package.json#L336
  4. nvm use
  5. yarn install
  6. yarn build:testing
  7. watch logs for duplicate plugins.

Environment

stof commented 2 years ago

@amplitude/utils is not part of the new Amplitude SDK maintained in this repo. It is part of https://github.com/amplitude/Amplitude-Node

The new @amplitude/analytics-node SDK available in this repo does not depend on @amplitude/utils at all (and it uses tslib 2)

stof commented 2 years ago

And amplitude-js is the old Amplitude JS SDK, available at https://github.com/amplitude/Amplitude-JavaScript

But I would say that the best solution would be to migrate to the new SDK in your project (using @amplitude/analytics-browser for the browser SDK).

kevinpagtakhan commented 2 years ago

I think the packages in this repo has tslib@2.x already, right?

stof commented 2 years ago

@kevinpagtakhan yes, this new SDK already uses tslib@2.x. but the description of the issue shows usages of the old SDK instead.

kevinpagtakhan commented 2 years ago

@stof so for us to help on this issue, we need to update amplitude-js dependency on @amplitude/utils to be the one we just released right? just making sure we're on the same page since there may be some confusion as to why this was opened on this repo

kevinpagtakhan commented 2 years ago

Can you check if this satisfies your proposed changes? https://github.com/amplitude/Amplitude-JavaScript/pull/553 Though I noticed amplitude-js has a yarn.lock. Not sure if you are able to dedupe despite of

stof commented 2 years ago

@kevinpagtakhan I'm not the one having the issue. But that's indeed my understanding.

kopax-polyconseil commented 2 years ago

@stof so for us to help on this issue

Thank you, we took not of the necessity to upgrade but it will have to be scheduled later in our backlogs, if we can have a quick fix that would be cool.

we need to update amplitude-js dependency on @amplitude/utils to be the one we just released right?

And yes you are right.

Though I noticed amplitude-js has a yarn.lock

Distributed module should not have lock file.

stof commented 2 years ago

the amplitude-js package does not have a lock file. the development repo has one, but it is not part of the npm package.

kevinpagtakhan commented 2 years ago

I see, so if lock files does not make it to npm then the PR should be sufficient

kevinpagtakhan commented 2 years ago

This is fixed in amplitude-js@v8.20.1