-
Notifications
You must be signed in to change notification settings - Fork 904
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add extension package that strips external JS loading #7766
Conversation
|
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1This report is too large (92,343 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.Test Logs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So sorry this is so complicated but you'll have to add the firebase
entry points as well. That means, make a copy of the packages/firebase/cordova
directory (index.ts and package.json), and add an entry in packages/firebase/package.json
copying cordova here: https://github.com/firebase/firebase-js-sdk/blob/master/packages/firebase/package.json#L74. These can be an exact copy of cordova (with the word "extension") subbed in as it's all self-referencing within packages/firebase
and not related to the bundles and paths you have created within the auth package. So you don't have to worry about extension-cjs
and extension-esm2017
etc.
Thanks Christina! I addressed your comments and added an extension entry point to firebase/auth. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except for a couple of small comments.
packages/auth/extension/package.json
Outdated
@@ -0,0 +1,8 @@ | |||
{ | |||
"name": "@firebase/auth/extension", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe call this "chrome-extension"? extension would be interpreted to mean it is some additional auth functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack. Renamed 'extension' to 'chrome-extension' here and other places (including folders' names). Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
web-extension
would be the preferred nomenclature. Since I presume we would want these to work in firefox and safari
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While Firebase Auth only supports Chrome extensions currently, I changed it to 'web-extension' in case we support other web extensions in the future. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
absolutely understand, appreciate the update
packages/auth/index.extension.ts
Outdated
|
||
import { ClientPlatform } from './src/core/util/version'; | ||
|
||
import { browserSessionPersistence } from './src/platform_browser/persistence/session_storage'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
session storage does not work in chrome extension(https://developer.chrome.com/docs/extensions/reference/api/storage#usage only mentions local storage and doesn't recommend it). we could remove this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack. Removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW - you can't use localStorage by default, since it doesn't exist in service workers, but you can use chrome.storage
, which does have session storage.
97bde31
to
ff46266
Compare
gapiScript: 'https://apis.google.com/js/api.js', | ||
recaptchaV2Script: 'https://www.google.com/recaptcha/api.js', | ||
recaptchaEnterpriseScript: | ||
'https://www.google.com/recaptcha/enterprise.js?render=' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is ?render=
needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I copied the exact url from https://github.com/firebase/firebase-js-sdk/blob/master/packages/auth/src/platform_browser/recaptcha/recaptcha_enterprise_verifier.ts#L35.
The full url we need to pass in _loadJS is 'https://www.google.com/recaptcha/enterprise.js?render=SITE-KEY'.
So if we remove '?render=' from here, we will have to update this line to be
url += '?render=' + siteKey;
…sm5 from rollup file
9fcd5d8
to
26634a4
Compare
Add an Extension package for developers to use when building a Chrome extension app.
This package strips the external JS loading to avoid violating Chrome Web Store's policy about remotely hosted code.
https://developer.chrome.com/docs/extensions/develop/migrate/improve-security#remove-remote-code