Skip to content
This repository was archived by the owner on Jul 9, 2025. It is now read-only.

fix: check for node on app start on users machine and notify if not installed #6578

Merged
pavolum merged 22 commits into from
Apr 8, 2021
Merged

fix: check for node on app start on users machine and notify if not installed #6578

pavolum merged 22 commits into from
Apr 8, 2021

Conversation

@pavolum
Copy link
Contributor

@pavolum pavolum commented Mar 26, 2021

Description

Adding server call that runs 'node -v' on the users machine. If the user does not have node the code catches the error and responds to the caller that node is not installed. If the user does have node it returns to the caller that node is installed along with he node version.

Added caller on app start to new server endpoint. If it returns that node is not installed it prompts the user with a modal on app start with a link to the node installation page.

Task Item

fixes #6097

Screenshots

image

@pavolum
Copy link
Contributor Author

pavolum commented Mar 26, 2021

OPEN QUESTIONS:

  • The modal allows the user to dismiss the modal and continue using the app even if node is not installed, should this be allowed or if node is not installed should we prevent further interaction with the app even though main app components like creation will not work
    • If we do let them navigate the app then maybe e should add some sort of banner to remind them that the app will have issues since node is not installed. At current state they will get an informative error if they run creation without Node, maybe that will suffice.

@emivers8
Copy link

emivers8 commented Mar 26, 2021

OPEN QUESTIONS:

  • The modal allows the user to dismiss the modal and continue using the app even if node is not installed, should this be allowed or if node is not installed should we prevent further interaction with the app even though main app components like creation will not work

    • If we do let them navigate the app then maybe e should add some sort of banner to remind them that the app will have issues since node is not installed. At current state they will get an informative error if they run creation without Node, maybe that will suffice.

Yeah, I wasn't sure what would be standard practice here. If we let it stay open, your suggestion is probably best. We don't want to have to throw up an error dialog every time someone attempts to use a feature that isn't going to work or be surprised when it crashes or something. Maybe @cwhitten can weigh in--would you expect the app to just close if you didn't install node, or would you expect to still use it, just with a banner as @pavolum suggests?

hatpick
hatpick reviewed Mar 26, 2021
View reviewed changes
@coveralls
Copy link

coveralls commented Mar 27, 2021 •
edited
Loading

Coverage Status

Coverage increased (+0.01%) to 51.154% when pulling 56028de on pavolum/addNodeModal into d2ae6bd on main.

@boydc2014
Copy link
Contributor

boydc2014 commented Apr 1, 2021 •
edited
Loading

I think we should not allow user to continue, if node is not found.

Also, i'm not sure the approach to detect node. If i understand correctly, this is for electron, right? because if it's web version, without node, the server won't even run. Even for electron, the underlying there is a node there to start server. So it's bit weird to detect node using node.

From a less diverged code path POV, I would prefer a simpler approach, like adding some scripts in electron startup and abort when no node is detected.

@cwhitten
Copy link
Member

cwhitten commented Apr 2, 2021 •
edited
Loading

  1. Node isn't required for the PVA scenarios. We definitely should not block use of Composer. The node requirement is a constraint to create new runtimes in Composer, not use Composer entirely. The two are different and we should treat it as such. I'd recommend we throw up the Node error when you try and step through the creation experience and we aren't in the PVA scenarios.
  2. As far as node recognizing node, it is weird I agree but generally if you replace node in this sense with python (if/when the SDK supported declarative/runtime) we would be using node to detect python, which makes a lot more sense.

My two cents would be to let users click around Composer, but not let them create new bots.

hatpick
hatpick previously approved these changes Apr 7, 2021
View reviewed changes
@cwhitten cwhitten added this to the R13 milestone Apr 7, 2021
@pavolum pavolum merged commit 72dde8a into main Apr 8, 2021
@pavolum pavolum deleted the pavolum/addNodeModal branch April 8, 2021 22:27
alanlong9278 added a commit that referenced this pull request Apr 9, 2021
@alanlong9278
Merge branch 'wenyluo/skill' into julong/skill
313b668 
* wenyluo/skill: (30 commits)
  Force refresh of fluent List via slice (#6810)
  fix: Telemetry for Orchestrator R13 (#6689)
  fix: Fixed broken tenant ID on save (#6809)
  feat: Add devops and app insights to get started (#6790)
  fix: check for node on app start on users machine and notify if not installed (#6578)
  feat: Expose default search query, pre-release flag and type for each feed. (#6781)
  fix: do not provision an endpoint key (#6791)
  remove folder if project fails to create (#6793)
  Removed preview from publish type names (#6795)
  fix: Fixing deep link for QNA and Luis (#6743)
  Make LUIS, QNA and Speech blocking modals so they cannot be dismiessed while active (#6788)
  Added workerRuntime value (#6784)
  fix: apply publishing profile to new runtime during publish (#6719)
  fix: during publish, do not copy project into a build folder (#6729)
  chore: Adapters: copy the 'type' passed in the adapter config to fully support adapter schemas (#6697)
  polish UI
  remove trigger when remove skill, fix bugs
  fix orchestractor modal not show
  fix skill not added after add remote skill
  remove allowedCaller when remove skill
  ...
alanlong9278 added a commit that referenced this pull request Apr 9, 2021
@alanlong9278
Merge branch 'main' into qika/skill
318c7d4 
* main:
  fix: make composer support https proxy (#6766)
  Force refresh of fluent List via slice (#6810)
  fix: Telemetry for Orchestrator R13 (#6689)
  fix: Fixed broken tenant ID on save (#6809)
  feat: Add devops and app insights to get started (#6790)
  fix: check for node on app start on users machine and notify if not installed (#6578)
  feat: Expose default search query, pre-release flag and type for each feed. (#6781)
  fix: do not provision an endpoint key (#6791)
  remove folder if project fails to create (#6793)
  Removed preview from publish type names (#6795)
  fix: Fixing deep link for QNA and Luis (#6743)
  Make LUIS, QNA and Speech blocking modals so they cannot be dismiessed while active (#6788)
  Added workerRuntime value (#6784)
  fix: apply publishing profile to new runtime during publish (#6719)
  fix: during publish, do not copy project into a build folder (#6729)
  chore: Adapters: copy the 'type' passed in the adapter config to fully support adapter schemas (#6697)
@cwhitten cwhitten mentioned this pull request May 20, 2021
Merged
benbrown pushed a commit to benbrown/BotFramework-Composer that referenced this pull request May 24, 2021
@alanlong9278
Merge branch 'wenyluo/skill' into julong/skill
c0e16c0 
* wenyluo/skill: (30 commits)
  Force refresh of fluent List via slice (microsoft#6810)
  fix: Telemetry for Orchestrator R13 (microsoft#6689)
  fix: Fixed broken tenant ID on save (microsoft#6809)
  feat: Add devops and app insights to get started (microsoft#6790)
  fix: check for node on app start on users machine and notify if not installed (microsoft#6578)
  feat: Expose default search query, pre-release flag and type for each feed. (microsoft#6781)
  fix: do not provision an endpoint key (microsoft#6791)
  remove folder if project fails to create (microsoft#6793)
  Removed preview from publish type names (microsoft#6795)
  fix: Fixing deep link for QNA and Luis (microsoft#6743)
  Make LUIS, QNA and Speech blocking modals so they cannot be dismiessed while active (microsoft#6788)
  Added workerRuntime value (microsoft#6784)
  fix: apply publishing profile to new runtime during publish (microsoft#6719)
  fix: during publish, do not copy project into a build folder (microsoft#6729)
  chore: Adapters: copy the 'type' passed in the adapter config to fully support adapter schemas (microsoft#6697)
  polish UI
  remove trigger when remove skill, fix bugs
  fix orchestractor modal not show
  fix skill not added after add remote skill
  remove allowedCaller when remove skill
  ...
benbrown pushed a commit to benbrown/BotFramework-Composer that referenced this pull request May 24, 2021
@alanlong9278
Merge branch 'main' into qika/skill
016a223 
* main:
  fix: make composer support https proxy (microsoft#6766)
  Force refresh of fluent List via slice (microsoft#6810)
  fix: Telemetry for Orchestrator R13 (microsoft#6689)
  fix: Fixed broken tenant ID on save (microsoft#6809)
  feat: Add devops and app insights to get started (microsoft#6790)
  fix: check for node on app start on users machine and notify if not installed (microsoft#6578)
  feat: Expose default search query, pre-release flag and type for each feed. (microsoft#6781)
  fix: do not provision an endpoint key (microsoft#6791)
  remove folder if project fails to create (microsoft#6793)
  Removed preview from publish type names (microsoft#6795)
  fix: Fixing deep link for QNA and Luis (microsoft#6743)
  Make LUIS, QNA and Speech blocking modals so they cannot be dismiessed while active (microsoft#6788)
  Added workerRuntime value (microsoft#6784)
  fix: apply publishing profile to new runtime during publish (microsoft#6719)
  fix: during publish, do not copy project into a build folder (microsoft#6729)
  chore: Adapters: copy the 'type' passed in the adapter config to fully support adapter schemas (microsoft#6697)
benbrown pushed a commit that referenced this pull request Jun 11, 2021
@alanlong9278
Merge branch 'wenyluo/skill' into julong/skill
2504e8b 
* wenyluo/skill: (30 commits)
  Force refresh of fluent List via slice (#6810)
  fix: Telemetry for Orchestrator R13 (#6689)
  fix: Fixed broken tenant ID on save (#6809)
  feat: Add devops and app insights to get started (#6790)
  fix: check for node on app start on users machine and notify if not installed (#6578)
  feat: Expose default search query, pre-release flag and type for each feed. (#6781)
  fix: do not provision an endpoint key (#6791)
  remove folder if project fails to create (#6793)
  Removed preview from publish type names (#6795)
  fix: Fixing deep link for QNA and Luis (#6743)
  Make LUIS, QNA and Speech blocking modals so they cannot be dismiessed while active (#6788)
  Added workerRuntime value (#6784)
  fix: apply publishing profile to new runtime during publish (#6719)
  fix: during publish, do not copy project into a build folder (#6729)
  chore: Adapters: copy the 'type' passed in the adapter config to fully support adapter schemas (#6697)
  polish UI
  remove trigger when remove skill, fix bugs
  fix orchestractor modal not show
  fix skill not added after add remote skill
  remove allowedCaller when remove skill
  ...
benbrown pushed a commit that referenced this pull request Jun 11, 2021
@alanlong9278
Merge branch 'main' into qika/skill
cbeeeee 
* main:
  fix: make composer support https proxy (#6766)
  Force refresh of fluent List via slice (#6810)
  fix: Telemetry for Orchestrator R13 (#6689)
  fix: Fixed broken tenant ID on save (#6809)
  feat: Add devops and app insights to get started (#6790)
  fix: check for node on app start on users machine and notify if not installed (#6578)
  feat: Expose default search query, pre-release flag and type for each feed. (#6781)
  fix: do not provision an endpoint key (#6791)
  remove folder if project fails to create (#6793)
  Removed preview from publish type names (#6795)
  fix: Fixing deep link for QNA and Luis (#6743)
  Make LUIS, QNA and Speech blocking modals so they cannot be dismiessed while active (#6788)
  Added workerRuntime value (#6784)
  fix: apply publishing profile to new runtime during publish (#6719)
  fix: during publish, do not copy project into a build folder (#6729)
  chore: Adapters: copy the 'type' passed in the adapter config to fully support adapter schemas (#6697)
lei9444 pushed a commit to lei9444/BotFramework-Composer-1 that referenced this pull request Jun 15, 2021
@pavolum @pavolumMsft
@benbrown @hatpick @cwhitten
fix: check for node on app start on users machine and notify if not i…
53d5be5 
…nstalled (microsoft#6578)

* Adding modal for node detection on app start

* - Adding server side checking of node
- Adding node modal on app start if node is not installed

* Moving node check to creation flow as opposed to app start

* Add isElectron check for node modal

Co-authored-by: Patrick Volum <pavolum@microsoft.com>
Co-authored-by: Ben Brown <benbro@microsoft.com>
Co-authored-by: Soroush <hatpick@gmail.com>
Co-authored-by: Chris Whitten <christopher.whitten@microsoft.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@a-b-r-o-w-n a-b-r-o-w-n Awaiting requested review from a-b-r-o-w-n

@beyackle beyackle Awaiting requested review from beyackle

@boydc2014 boydc2014 Awaiting requested review from boydc2014

@GeoffCoxMSFT GeoffCoxMSFT Awaiting requested review from GeoffCoxMSFT

@srinaath srinaath Awaiting requested review from srinaath

@tonyanziano tonyanziano Awaiting requested review from tonyanziano

2 more reviewers

@cwhitten cwhitten cwhitten approved these changes

@hatpick hatpick hatpick left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@cwhitten cwhitten

@hatpick hatpick

Labels

None yet

Projects

None yet

Milestone

R13

Development

Successfully merging this pull request may close these issues.

NPM and Node.js not installed needs to prompt you to install Node.js and NPM

9 participants

@pavolum @emivers8 @coveralls @boydc2014 @cwhitten @hatpick @pavolumMsft @benbrown