Added ES module bundle to the auth package by merlinnot · Pull Request #819 · firebase/firebase-js-sdk

merlinnot · 2018-05-14T22:24:07Z

Resolves #818

There is no changelog entry 'cause I couldn't find it ;)

merlinnot · 2018-05-15T16:37:45Z

Travis probably needs a retry.

merlinnot · 2018-05-16T16:58:12Z

May I kindly ask for a review?

bojeil-google · 2018-05-18T07:54:33Z

Hey @merlinnot, thanks for your contribution. I reran the tests several times and they are all failing. Please fix and I can help review.

merlinnot · 2018-05-18T08:47:03Z

I will take a look in the evening.

merlinnot · 2018-05-21T11:37:06Z

Travis build: #818 (comment)

Issue was caused by getting a PID of a command creation, rather than the command itself.

The new name should better reflect the purpose of a function.

merlinnot · 2018-06-05T18:21:01Z

@bojeil-google test are passing, I'd appreciate a review :)

bojeil-google

Changes look good. I just have a bunch of requests for additional explanations and some style nits.

bojeil-google · 2018-06-06T01:14:55Z

packages/auth/buildtools/run_tests.sh

+  "./node_modules/.bin"
+  "../../node_modules/.bin"
+)
+function evalModule {


Declare javadoc or some guidance on this function's usage/signature.

bojeil-google · 2018-06-06T01:15:50Z

packages/auth/buildtools/run_tests.sh

+declare -a nodeModulesBasedirs=(
+  "./node_modules/.bin"
+  "../../node_modules/.bin"
+)


Add new line after this

bojeil-google · 2018-06-06T01:18:52Z

packages/auth/gulpfile.js

-}).call(typeof global !== 'undefined' ? global : typeof self !== 'undefined' ? self : typeof window !== 'undefined' ? window : {});`;
+const CJS_WRAPPER_PREFIX = `(function() {var firebase = require('@firebase/app').default;`;
+const EMS_WRAPPER_PREFIX = `import firebase from '@firebase/app';(function() {`;
+const WRAPPER_SUFFIX = `}).call(typeof global !== 'undefined' ? global : typeof self !== 'undefined' ? self : typeof window !== 'undefined' ? window : {});`;


Line is too long. Split into multiple lines.

bojeil-google · 2018-06-06T01:22:13Z

packages/auth/gulpfile.js


-  app.use('/node_modules', express.static(path.resolve(__dirname, '../../node_modules')));
+  app.use(
+    '/node_modules',


indent 2 additional spaces. Style guide recommendations is to indent 4 spaces for function arguments that spill over to next line.

bojeil-google · 2018-06-06T01:22:23Z

packages/auth/gulpfile.js

-  app.use('/node_modules', express.static(path.resolve(__dirname, '../../node_modules')));
+  app.use(
+    '/node_modules',
+    express.static(path.resolve(__dirname, '../../node_modules'))


indent 2 additional spaces.

bojeil-google · 2018-06-06T01:22:50Z

packages/auth/gulpfile.js

-const closureLibRoot = path.dirname(require.resolve('google-closure-library/package.json'));
-// Builds the core Firebase-auth JS.
+const closureLibRoot = path.dirname(
+  require.resolve('google-closure-library/package.json')


indent 2 additional spaces.

bojeil-google · 2018-06-06T01:23:43Z

packages/auth/gulpfile.js

-      }
-    }))
+    .pipe(
+      closureCompiler({


indent entire block 2 additional spaces.

bojeil-google · 2018-06-06T01:27:54Z

packages/auth/gulpfile.js

+            'externs/grecaptcha.js',
+            'externs/gapi.iframes.js',
+            path.resolve(
+              __dirname,


Both arguments need 2 additional spaces, here and below for each path.resolve(.

bojeil-google · 2018-06-06T01:29:54Z

packages/auth/gulpfile.js

+const EMS_WRAPPER_PREFIX = `import firebase from '@firebase/app';(function() {`;
+const WRAPPER_SUFFIX = `}).call(typeof global !== 'undefined' ? global : typeof self !== 'undefined' ? self : typeof window !== 'undefined' ? window : {});`;
+
+const wrap = through(function(file) {


Add comment or javadoc to explain what this function does.

merlinnot · 2018-06-08T23:53:35Z

Thanks for the review, gonna take care of it tomorrow.

test script.

package's gulpfile.

comments.

merlinnot · 2018-06-09T11:03:34Z

@bojeil-google sorry for not aligning with a style guide, it's hard to match all of the criteria when you use other standards on a daily basis. I've fixed the issues you've mentioned and one additional call to align with the rule you've pointed out.

bojeil-google

Thanks for making the changes! 2 more nits. You are right we should document our style guide. We'll look into it.

bojeil-google · 2018-06-09T16:29:00Z

packages/auth/gulpfile.js

 // For minified builds, wrap the output so we avoid leaking global variables.
-const CJS_WRAPPER_PREFIX = `(function() {var firebase = require('@firebase/app').default;`;
+const CJS_WRAPPER_PREFIX =
+  `(function() {var firebase = require('@firebase/app').default;`;


Add 2 more spaces here. (if right hand side of = is on second line)

const CJS_WRAPPER_PREFIX = 'value';

bojeil-google · 2018-06-09T16:29:23Z

packages/auth/gulpfile.js

+const WRAPPER_SUFFIX =
+  `}).call(typeof global !== 'undefined' ? ` +
+  `global : typeof self !== 'undefined' ? ` +
+  `self : typeof window !== 'undefined' ? window : {});`;


Same here for above block:
Add 2 more spaces here. (if right hand side of = is on second line)

const WRAPPER_SUFFIX = 'value1' + 'value2';

auth package's gulpfile.

merlinnot · 2018-06-09T18:11:39Z

It is actually documented here, the thing that is missing is an auto-formatting tool that would check for all of these rules. It's enough to have a documentation for people who work on the project on a daily basis, but it's hard to expect from external contributors to learn all of theses rules to make a few small PRs, it's just a waste of time.

A tool like Prettier (aren't you already using it in other packages?) or clang would do.

Thank you for the approval!

Added ES module build to the auth package.

9841275

merlinnot requested review from bojeil-google and wti806 as code owners May 14, 2018 22:24

google-oss-bot added the needs-triage label May 14, 2018

merlinnot mentioned this pull request May 14, 2018

es6 import fails with module-aware browsers #100

Closed

Merge branch 'master' into fix-issue-818

90a4f7a

merlinnot added 2 commits May 18, 2018 20:28

[auth] Improved build efficiency by reusing Clousure Compiler output.

790b923

Merge branch 'master' into fix-issue-818

b9337e2

merlinnot requested review from mikelehen and schmidt-sebastian as code owners May 18, 2018 20:24

[auth] Improved a test script to accomodate hoisting.

311c585

merlinnot force-pushed the fix-issue-818 branch from da556ea to 311c585 Compare May 18, 2018 20:46

[AUTOMATED]: Prettier Code Styling

808f114

merlinnot force-pushed the fix-issue-818 branch from 7b50cb9 to 808f114 Compare May 19, 2018 06:38

merlinnot added 5 commits May 21, 2018 11:37

Merge branch 'master' into fix-issue-818

2895d0e

Merge branch 'master' into fix-issue-818

141c880

Removed build step from a test script in the auth package.

538acd6

Fixed an issue with getting a PID of a local server in auth tests.

316b170

Issue was caused by getting a PID of a command creation, rather than the command itself.

Changed a local method name in a gulp taks of an auth package.

1b43e00

The new name should better reflect the purpose of a function.

Merge branch 'master' into fix-issue-818

66b6d95

bojeil-google suggested changes Jun 6, 2018

View reviewed changes

bojeil-google added api: auth and removed needs-triage labels Jun 6, 2018

merlinnot mentioned this pull request Jun 6, 2018

Firestore FirebaseExtended/polymerfire#327

Open

merlinnot added 4 commits June 9, 2018 12:23

Improved a documentation, spacing and prevented duplicate calls in auth

a7492b6

test script.

Formatted constants in auth package's gulpfile to fit in 80 columns.

37b70fb

Added a comment to explain the purpose of a wrap method used in auth

b738893

package's gulpfile.

Reformatted a gulpfile in the auth package according to reviewer's

304b1fc

comments.

bojeil-google approved these changes Jun 9, 2018

View reviewed changes

Added spaces to multiline assignments to align with a style guide in the

aaee39a

auth package's gulpfile.

bojeil-google approved these changes Jun 11, 2018

View reviewed changes

bojeil-google merged commit f2a4472 into firebase:master Jun 11, 2018

merlinnot deleted the fix-issue-818 branch June 11, 2018 10:08

firebase locked and limited conversation to collaborators Oct 20, 2019

Conversation

merlinnot commented May 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

merlinnot commented May 15, 2018

Uh oh!

merlinnot commented May 16, 2018

Uh oh!

bojeil-google commented May 18, 2018

Uh oh!

merlinnot commented May 18, 2018

Uh oh!

merlinnot commented May 21, 2018

Uh oh!

merlinnot commented Jun 5, 2018

Uh oh!

bojeil-google 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!

Choose a reason for hiding this comment

Uh oh!

merlinnot commented Jun 8, 2018

Uh oh!

merlinnot commented Jun 9, 2018

Uh oh!

bojeil-google 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!

merlinnot commented Jun 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

merlinnot commented May 14, 2018 •

edited

Loading