Fix #248 by adding a new option deferInitialization #1303

seratch · 2022-02-04T03:07:09Z

Summary

This pull request resolves #248 by adding deferInitialization option along with App#init() method, which enable developers to take full control of the timing to run the initialization steps that require async/await code executions.

JavaScript/TypeScript does not provide any ways to run async functions inside object constructors. That's why the issue #248 has not been resolved for a long time.

This pull request proposes a new option to customize the behavior to overcome the limitation. With the new way, developers can catch the exception that can be thrown by App#init() method:

const { App } = require('@slack/bolt');

const app = new App({
  token: process.env.SLACK_BOT_TOKEN,
  signingSecret: process.env.SLACK_SIGNING_SECRET,
  deferInitialization: true,  // The default value is still false
});

(async () => {
  try {
    await app.init();  // may throw an exception if the given token is invalid
    await app.start(process.env.PORT || 3000);
  } catch (e) {
    console.error(e);
    process.exit(255);
  }
  console.log('⚡️ Bolt app is running!');
})();

Requirements (place an `x` in each `[ ]`)

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

codecov · 2022-02-04T03:08:40Z

Codecov Report

Merging #1303 (ebb7da2) into main (bbb2553) will decrease coverage by 0.35%.
The diff coverage is 70.83%.

@@            Coverage Diff             @@
##             main    #1303      +/-   ##
==========================================
- Coverage   73.24%   72.88%   -0.36%     
==========================================
  Files          17       17              
  Lines        1439     1479      +40     
  Branches      431      442      +11     
==========================================
+ Hits         1054     1078      +24     
- Misses        300      311      +11     
- Partials       85       90       +5

Impacted Files	Coverage Δ
src/App.ts	`82.71% <70.83%> (-2.35%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bbb2553...ebb7da2. Read the comment docs.

seratch · 2022-02-04T03:08:39Z

src/App.ts

@@ -105,6 +105,7 @@ export interface AppOptions {
  socketMode?: boolean;
  developerMode?: boolean;
  tokenVerificationEnabled?: boolean;
+  deferInitialization?: boolean;


Naming suggestions?

This naming makes the most sense to me.

I think this is fine

srajiang

This is such a nice addition @seratch!

I tested your branch against a sample app, and noticed that when deferInitialization set as true but still omitting await app.init(); there is no error message for devs.

Do you think we should be enforcing that if deferInitialization is true, the Bolt App should not be starting unless the user has manually called init()?

srajiang · 2022-02-04T18:14:03Z

src/App.ts

@@ -105,6 +105,7 @@ export interface AppOptions {
  socketMode?: boolean;
  developerMode?: boolean;
  tokenVerificationEnabled?: boolean;
+  deferInitialization?: boolean;


This naming makes the most sense to me.

src/App.ts

srajiang · 2022-02-04T18:17:07Z

src/App.ts

-        installerOptions: this.installerOptions,
-      });
-    }
+    this.receiver = this.initReceiver(


This is so nice! 💯

Agreed, I like splitting up the initialization into discrete functions!

filmaj

This is great, thanks for tackling this @seratch !

Not sure if we should bundle the documentation for this feature in this PR, or a separate one, but I think before merging this PR we should at least write a draft of the docs for this support. In my experience, writing up the docs alongside (or even before) the code is written up can help clarify how to structure the code too. I am happy to help write the docs if you would find that helpful - let me know, I can try to create a draft!

filmaj · 2022-02-04T18:22:09Z

src/App.ts

@@ -105,6 +105,7 @@ export interface AppOptions {
  socketMode?: boolean;
  developerMode?: boolean;
  tokenVerificationEnabled?: boolean;
+  deferInitialization?: boolean;


I think this is fine

filmaj · 2022-02-04T18:27:01Z

src/App.ts

-        installerOptions: this.installerOptions,
-      });
-    }
+    this.receiver = this.initReceiver(


Agreed, I like splitting up the initialization into discrete functions!

src/App.ts

seratch · 2022-02-04T22:03:02Z

@srajiang Thanks for the great suggestion! After submitting this PR, I was also thinking about the point. Will add a validation in the #start() method.
@filmaj Thanks! I've created a task issue for the document work. If you have the bandwidth, helping me on the doc would be appreciated! #1304

seratch · 2022-02-04T23:20:18Z

@filmaj @srajiang I've revised all the points in your reviews 👋

seratch · 2022-02-04T23:20:45Z

src/App.spec.ts

+      // Assert
+      assert.instanceOf(app, MockApp);
+      try {
+        // call #start() before #init()


Added this test pattern

seratch · 2022-02-04T23:21:16Z

examples/custom-properties/package.json

  "scripts": {
    "ngrok": "ngrok http 3000",
-    "start": "node app.js",
+    "start": "node http.js",


This has been incorrect since I've renamed the source file.

seratch · 2022-02-04T23:22:03Z

examples/custom-properties/http.js

-
+  try {
+    await app.init();
+    await app.start(process.env.PORT || 3000);


If you don't call init() in advance, this start method fails this way:

$ npm start > bolt-js-custom-properties-app@1.0.0 start /Users/ksera/github/bolt-js/examples/custom-properties > node http.js AppInitializationError: This App instance is not yet initialized. Call `await App#init()` before starting the app. at App.start (/Users/ksera/github/bolt-js/dist/App.js:239:19) at /Users/ksera/github/bolt-js/examples/custom-properties/http.js:46:15 at Object.<anonymous> (/Users/ksera/github/bolt-js/examples/custom-properties/http.js:52:3) at Module._compile (internal/modules/cjs/loader.js:1158:30) at Object.Module._extensions..js (internal/modules/cjs/loader.js:1178:10) at Module.load (internal/modules/cjs/loader.js:1002:32) at Function.Module._load (internal/modules/cjs/loader.js:901:14) at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:74:12) at internal/main/run_main_module.js:18:47 { code: 'slack_bolt_app_initialization_error' } npm ERR! code ELIFECYCLE npm ERR! errno 255 npm ERR! bolt-js-custom-properties-app@1.0.0 start: `node http.js` npm ERR! Exit status 255 npm ERR!

seratch · 2022-02-04T23:22:22Z

examples/custom-properties/http.js

-  await app.start(process.env.PORT || 3000);
-
+  try {
+    await app.init();


Example error pattern:

> bolt-js-custom-properties-app@1.0.0 start /Users/ksera/github/bolt-js/examples/custom-properties > node http.js AppInitializationError: Apps used in a single workspace can be initialized with a token. Apps used in many workspaces should be initialized with oauth installer options or authorize. Since you have not provided a token or authorize, you might be missing one or more required oauth installer options. See https://slack.dev/bolt-js/concepts#authenticating-oauth for these required fields. at App.initAuthorizeIfNoTokenIsGiven (/Users/ksera/github/bolt-js/dist/App.js:660:19) at App.init (/Users/ksera/github/bolt-js/dist/App.js:180:47) at /Users/ksera/github/bolt-js/examples/custom-properties/http.js:46:15 at Object.<anonymous> (/Users/ksera/github/bolt-js/examples/custom-properties/http.js:53:3) at Module._compile (internal/modules/cjs/loader.js:1158:30) at Object.Module._extensions..js (internal/modules/cjs/loader.js:1178:10) at Module.load (internal/modules/cjs/loader.js:1002:32) at Function.Module._load (internal/modules/cjs/loader.js:901:14) at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:74:12) at internal/main/run_main_module.js:18:47 { code: 'slack_bolt_app_initialization_error' } npm ERR! code ELIFECYCLE npm ERR! errno 255 npm ERR! bolt-js-custom-properties-app@1.0.0 start: `node http.js` npm ERR! Exit status 255

seratch · 2022-02-04T23:23:01Z

src/App.ts

@@ -514,6 +514,11 @@ export default class App {
  public start(
    ...args: Parameters<HTTPReceiver['start'] | SocketModeReceiver['start']>
  ): ReturnType<HTTPReceiver['start']> {
+    if (!this.initialized) {
+      throw new AppInitializationError(
+        'This App instance is not yet initialized. Call `await App#init()` before starting the app.',


Any suggestions on the error message?

This is great!

src/App.spec.ts

Co-authored-by: Sarah Jiang <srajiang@gmail.com>

srajiang

Thanks for turning around these changes! It's looking good to me :)

filmaj

Thanks for making the changes! Looks good to me

And most definitely I will help with the docs, added to my todo list for this week. I will work on a PR for that.

seratch · 2022-02-14T05:31:46Z

Thanks for the reviews! Let me merge this PR now.

seratch added 2 commits February 4, 2022 11:56

Fix slackapi#248 by adding a new option deferInitialization

9edef5b

Move private methods in code

0a67221

seratch added the enhancement M-T: A feature request for new functionality label Feb 4, 2022

seratch added this to the 3.10.0 milestone Feb 4, 2022

seratch requested review from mwbrooks, filmaj, stevengill, misscoded and srajiang February 4, 2022 03:07

seratch self-assigned this Feb 4, 2022

seratch commented Feb 4, 2022

View reviewed changes

srajiang requested changes Feb 4, 2022

View reviewed changes

filmaj reviewed Feb 4, 2022

View reviewed changes

seratch mentioned this pull request Feb 4, 2022

Add a document section about deferInitialization option when releasing v3.10 #1304

Closed

seratch added 4 commits February 5, 2022 08:09

Improve a lot

cd57645

Update more

e837905

Fix

fa695ae

Fix examples

3b7b6aa

seratch commented Feb 4, 2022

View reviewed changes

srajiang reviewed Feb 4, 2022

View reviewed changes

src/App.spec.ts Outdated Show resolved Hide resolved

Update src/App.spec.ts

f4d52c9

Co-authored-by: Sarah Jiang <srajiang@gmail.com>

srajiang approved these changes Feb 4, 2022

View reviewed changes

filmaj approved these changes Feb 7, 2022

View reviewed changes

Merge branch 'main' into issue-248-auth-test-failure

ebb7da2

seratch merged commit 446622c into slackapi:main Feb 14, 2022

seratch deleted the issue-248-auth-test-failure branch February 14, 2022 05:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #248 by adding a new option deferInitialization #1303

Fix #248 by adding a new option deferInitialization #1303

seratch commented Feb 4, 2022 •

edited

Loading

codecov bot commented Feb 4, 2022 •

edited

Loading

seratch Feb 4, 2022

srajiang Feb 4, 2022

filmaj Feb 4, 2022

srajiang left a comment •

edited

Loading

srajiang Feb 4, 2022

srajiang Feb 4, 2022

filmaj Feb 4, 2022

filmaj left a comment

filmaj Feb 4, 2022

filmaj Feb 4, 2022

seratch commented Feb 4, 2022

seratch commented Feb 4, 2022

seratch Feb 4, 2022

seratch Feb 4, 2022

seratch Feb 4, 2022

seratch Feb 4, 2022

seratch Feb 4, 2022

srajiang Feb 4, 2022

srajiang left a comment

filmaj left a comment

seratch commented Feb 14, 2022

Fix #248 by adding a new option deferInitialization #1303

Fix #248 by adding a new option deferInitialization #1303

Conversation

seratch commented Feb 4, 2022 • edited Loading

Summary

Requirements (place an x in each [ ])

codecov bot commented Feb 4, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srajiang left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filmaj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seratch commented Feb 4, 2022

seratch commented Feb 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srajiang left a comment

Choose a reason for hiding this comment

filmaj left a comment

Choose a reason for hiding this comment

seratch commented Feb 14, 2022

seratch commented Feb 4, 2022 •

edited

Loading

Requirements (place an `x` in each `[ ]`)

codecov bot commented Feb 4, 2022 •

edited

Loading

srajiang left a comment •

edited

Loading