Skip to content
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 missing null unions for plain text and static select block action… #1915

Closed

Conversation

GovernmentHack
Copy link
Contributor

… types

Summary

Add the missing type definitions to fix Issue 1914 #1914

Requirements (place an x in each [ ])

@GovernmentHack GovernmentHack force-pushed the fix-for-issue-1914 branch 2 times, most recently from 2bfbef8 to 7ded116 Compare July 31, 2023 22:01
@GovernmentHack
Copy link
Contributor Author

Hmm, I'm unsure why the build is failing 🤔 Seems to be because of an import order on a file I haven't touched as part of the PR. I did just get #1910 approved and merged based on the same state of main, but I didn't see if that also had the same failing builds.

@filmaj
Copy link
Contributor

filmaj commented Aug 1, 2023

@GovernmentHack I believe it was the change that was introduced in #1910 that is causing this failure. main is failing for me locally. Looks like our tests need updating as now there is a new required username property via #1910.

@GovernmentHack
Copy link
Contributor Author

@GovernmentHack I believe it was the change that was introduced in #1910 that is causing this failure. main is failing for me locally. Looks like our tests need updating as now there is a new required username property via #1910.

Is #1910 the cause? the errors seem to indicate some sort of import error:

> @slack/bolt@3.13.2 test
> npm run lint && npm run mocha && npm run test:types


> @slack/bolt@3.13.2 lint
> eslint --fix --ext .ts src


/home/runner/work/bolt-js/bolt-js/src/receivers/AwsLambdaReceiver.spec.ts
Error:   5:1  error  `crypto` import should occur before import of `sinon`  import/order

✖ 1 problem (1 error, 0 warnings)

Error: Process completed with exit code 1.

@filmaj
Copy link
Contributor

filmaj commented Aug 1, 2023

@GovernmentHack #1910 was the cause for the unit tests to fail - if you run just the unit tests locally, you should be able to reproduce (at least, I was able to reproduce locally).
I just fixed the lint failures in #1916 and merged to main. If you can rebase this PR, we can slowly move forward with this PR. Appreciate your patience!

@GovernmentHack
Copy link
Contributor Author

@filmaj Sure thing! rebased and pushed. Thank you again for the help!

@codecov
Copy link

codecov bot commented Aug 1, 2023

Codecov Report

Merging #1915 (4add60c) into main (ba5b893) will not change coverage.
The diff coverage is n/a.

❗ Current head 4add60c differs from pull request most recent head 9c51b63. Consider uploading reports for the commit 9c51b63 to get more accurate results

@@           Coverage Diff           @@
##             main    #1915   +/-   ##
=======================================
  Coverage   82.18%   82.18%           
=======================================
  Files          18       18           
  Lines        1521     1521           
  Branches      436      436           
=======================================
  Hits         1250     1250           
  Misses        175      175           
  Partials       96       96           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@filmaj
Copy link
Contributor

filmaj commented Aug 2, 2023

@GovernmentHack thanks for the PR but first let's figure out what the issue is, I'd like to continue the conversation in #1914 before we make any additional moves or changes in this PR.

@GovernmentHack
Copy link
Contributor Author

Per our conversation in the issue, I'm good for this PR to be rejected 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants