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 readableFlowing property to INodeSocket #2515

Merged
merged 1 commit into from
Jul 10, 2020
Merged

Conversation

mdrichardson
Copy link
Contributor

@mdrichardson mdrichardson commented Jul 10, 2020

All JS builds are breaking I believe due to a linting or pipeline update(?); I didn't really see any other relevant recent commits. INodeSocket needs to implement readableFlowing so I added it as a property that does nothing. It can't be optional or the build will still break. It uses the same typing and scope as Socket defined in Node globals:

image

Here's the failure:

src/webSocket/nodeWebSocket.ts(39,69): error TS2345: Argument of type 'INodeSocket' is not assignable to parameter of type 'Socket'.
> botframework-config@4.4.0-preview.145306 build /Users/runner/work/1/s/libraries/botframework-config
  Property 'readableFlowing' is missing in type 'INodeSocket' but required in type 'Socket'.

The reference for the change doesn't amount to much more than "it was missing, so we added it". Similarly, this change isn't mentioned anywhere in Node release notes, as far as I can tell, and digging through the blame doesn't really explain anything, either.

@mdrichardson mdrichardson requested review from a team and stevengum and removed request for a team July 10, 2020 17:01
@coveralls
Copy link

coveralls commented Jul 10, 2020

Pull Request Test Coverage Report for Build 145317

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 80.849%

Totals Coverage Status
Change from base Build 145170: 0.0%
Covered Lines: 12957
Relevant Lines: 15308

💛 - Coveralls

@stevengum stevengum merged commit d79b10b into master Jul 10, 2020
@stevengum stevengum deleted the mdrichardson/fixBuild branch July 10, 2020 20:17
@chon219
Copy link
Member

chon219 commented Jul 13, 2020

I pulled the latest code from master branch and got the following error when I ran "npm run build", is there anything broken?

lerna ERR! npm run build exited 2 in 'botframework-streaming'
lerna ERR! npm run build stdout:

> [email protected] build /home/chon/workspace/botbuilder-js/libraries/botframework-streaming
> tsc -p tsconfig.json && tsc -p tsconfig-browser.json

src/namedPipe/namedPipeClient.ts(60,53): error TS2345: Argument of type 'Socket' is not assignable to parameter of type 'INodeSocket'.
  Property 'readableFlowing' is missing in type 'Socket' but required in type 'INodeSocket'.
src/namedPipe/namedPipeClient.ts(61,55): error TS2345: Argument of type 'Socket' is not assignable to parameter of type 'INodeSocket'.

@stevengum
Copy link
Member

@chon219, if you run lerna bootstrap —hoist again you’ll have the latest @types/node dependency which should resolve your build error.

@chon219
Copy link
Member

chon219 commented Jul 13, 2020

@chon219, if you run lerna bootstrap —hoist again you’ll have the latest @types/node dependency which should resolve your build error.

Thanks, I would try again.

@mdrichardson mdrichardson mentioned this pull request Jul 13, 2020
@chon219
Copy link
Member

chon219 commented Jul 15, 2020

@chon219, if you run lerna bootstrap —hoist again you’ll have the latest @types/node dependency which should resolve your build error.

I've updated @types/node dependency to 10.17.26 via running lerna bootstrap --hoist but still got the same build error, it's so weird.

@mdrichardson
Copy link
Contributor Author

@chon219 I just cloned and am not running into this. If you try cloning the repo into a new folder, do you still experience this? And you're using the botbuilder-js repo, right? Do you see the readableFlowing property in libraries/botframework-streaming/src/interfaces/INodeSocket.ts ?

@stevengum
Copy link
Member

@chon219 if you do a git clean -fdx at the root level and then do lerna bootstrap —hoist that will ensure the old node types and package-lock.jsons are gone.

@chon219
Copy link
Member

chon219 commented Jul 15, 2020

@mdrichardson yes, there is a readableFlowing property
image

I cloned a new copy of botbuilder-js and it builds well without errors, but I don't know why this would happen...

@chon219
Copy link
Member

chon219 commented Jul 15, 2020

@chon219 if you do a git clean -fdx at the root level and then do lerna bootstrap —hoist that will ensure the old node types and package-lock.jsons are gone.

Got it, thanks!

stevengum added a commit that referenced this pull request Nov 18, 2020
* add .yaml-based Azure DevOps pipelines to 4.8 branch

* add readableFlowing property to INodeSocket (#2515)

* fix cherry-pick 8d9a3e3

Co-authored-by: Michael Richardson <[email protected]>
stevengum added a commit that referenced this pull request Nov 29, 2020
* add .yaml-based Azure DevOps pipelines to 4.8 branch
* add readableFlowing property to INodeSocket (#2515)
* fix cherry-pick 8d9a3e3

Co-authored-by: Michael Richardson <[email protected]>
stevengum added a commit that referenced this pull request Nov 30, 2020
* cherry-pick (#3075) to 4.7, fix merge conflicts
* add .yaml-based Azure DevOps pipelines to 4.8 branch
* add readableFlowing property to INodeSocket (#2515)
* fix cherry-pick 8d9a3e3

Co-authored-by: Michael Richardson <[email protected]>

* change from adaptive-expressions to botframework-expressions
* manually pick tokenResolver test fixes
 * from f1b6fc2
* address PR feedback re: INodeSocket
* remove unused internal AddressInfo interface
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants