Import package version from package.json #1764
Conversation
mmermerkaya
requested review from
alikn,
andirayo,
dwoffinden,
Feiyang1,
hiranya911,
hsubox76 and
zijianjoy
as code owners
| @@ -15,6 +15,8 @@ | |||
| * limitations under the License. | |||
| */ | |||
|
|
|||
| import './setup'; | |||
packages/util/src/global.ts
Outdated
|
|
||
| /** The global object in both Node and browser environments. */ | ||
| export const root: any = | ||
| (typeof self === 'object' && self.self === self && self) || |
There was a problem hiding this comment.
I really like being type safe and eliminating typos, but It's unfortunate that it requires setting variables on global for testing. I feel this extra (hackish) step makes our code harder to understand.
How about we import the version number from package.json directly. We just need an extra rollup plugin and setting a typescript compiler flag. It has all the benefits, is easy to understand and the change is minimal.
I made a PR using this approach for @firebase/app - #1775 WDYT?
packages/app/rollup.config.js
Outdated
| values: { | ||
| JSCORE_VERSION: firebasePkg.version | ||
| JSCORE_VERSION: JSON.stringify(firebasePkg.version) |
There was a problem hiding this comment.
firebasePkg.version is already string. Isn't JSON.stringify redundant?
There was a problem hiding this comment.
Without stringify, the replacement would be the value of the string, as x.y.z. So the code would look like this: const version = 1.2.3;
With stringify, it's instead replaced with "x.y.z", so the example would be const version = "1.2.3";, which is what we want.
Rollup plugin example here: https://github.com/rollup/rollup-plugin-replace#usage
| @@ -28,7 +28,7 @@ import { createFirebaseNamespaceCore } from '../firebaseNamespaceCore'; | |||
| export function createFirebaseNamespaceLite(): FirebaseNamespace { | |||
| const namespace = createFirebaseNamespaceCore(FirebaseAppLiteImpl); | |||
|
|
|||
| namespace.SDK_VERSION = '${JSCORE_VERSION}_LITE'; | |||
| namespace.SDK_VERSION = `${__JSCORE_VERSION__}_LITE`; | |||
There was a problem hiding this comment.
I didn't want to change the original.
|
Yeah I agree about the testing stuff. I didn't realize that it would take so much effort to make it work until it was too late 😅 I like your version, thanks for that! I tried to keep the replace plugin in case we use it for something else, but all three packages that I touched only use it for the version string. YAGNI, I guess 😄 I'll merge your PR to my branch, and replace the others as well. |
* import package.json for version * [AUTOMATED]: Prettier Code Styling * use var in es5 build
mmermerkaya
force-pushed
the
mmermerkaya-standardize-replace
branch
from
3c7d84a to
f2bba9c
Compare
mmermerkaya
changed the title
|
Done. Edited OP as well. |
mmermerkaya
force-pushed
the
mmermerkaya-standardize-replace
branch
from
f2bba9c to
7410820
Compare
mmermerkaya
force-pushed
the
mmermerkaya-standardize-replace
branch
from
7410820 to
d0dca90
Compare
|
Looks like this changed the TypeScript output directory structure in App and Installations, because we added an import from a parent directory of the current top level directory in both of these packages. Should be fine with the |
Imports version strings directly from
package.json, which allows TS to do type checking and warn if there's a typo or a type mismatch. Removes the need forrollup-plugin-replace.