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

New uibuilder - cannot remove bootstrap and bootstrap-vue #75

Closed
bakman2 opened this issue Oct 4, 2019 · 19 comments
Closed

New uibuilder - cannot remove bootstrap and bootstrap-vue #75

bakman2 opened this issue Oct 4, 2019 · 19 comments
Assignees
Labels
Fixed Phew, think I got away with it. no-issue-activity Workaround Available Yup, probably a bug but there is a temporary workaround
Milestone

Comments

@bakman2
Copy link

bakman2 commented Oct 4, 2019

Issue

Cannot remove bootstrap and bootstrap-vue

Reproduce

Drag a ui-builder node to the flow window.
Doubleclick uibuilder node, click "Manage front-end libraries", click 'X' for both bootstrap and bootstrap-vue

What should happen ?

bootstrap and bootstrap-vue should be removed

What actually happens:

debug error

"[uibuilder/uibnpmmanage] Admin API. npm command failed. npm remove for package bootstrap"

Remark

If they cannot be removed, do not show them in the list. Possibly applies to vue as well.

System

Software Version
Node.JS 10.16.3
npm 6.11.3
Node-RED 1.0.0
uibuilder node 2.0.5
uibuilderFE ?
OS MacOS 10.14.6
Browser Safari 13.0.1
@TotallyInformation
Copy link
Owner

Well, they should be removable, so that should work!

I will investigate, thanks for reporting.

Not a critical bug of course since everything will carry on working just fine, simply remove the references from your front-end code. However, not efficient so I will certainly look to a fix.

@TotallyInformation TotallyInformation self-assigned this Oct 4, 2019
@TotallyInformation TotallyInformation added bug It's life Jim but not as we know it! Investigating What the ... !? Workaround Available Yup, probably a bug but there is a temporary workaround labels Oct 4, 2019
@github-actions
Copy link

github-actions bot commented Dec 4, 2019

Stale issue message

@TotallyInformation
Copy link
Owner

Still looking. Not currently top priority.

@T0T4R4
Copy link

T0T4R4 commented Dec 24, 2019

still occuring, maybe not top priority for you but high priority for those who simply don't want to rely on boostrap / boostrap-vue

@TotallyInformation
Copy link
Owner

still occuring, maybe not top priority for you but high priority for those who simply don't want to rely on boostrap / boostrap-vue

I’m certainly not dismissing the issue. Just have limited time right now. Unless you are really tight for space, you can simply ignore them. It really makes no difference to your front end.

I’m also happy to take PR to fix.

@github-actions
Copy link

Stale issue message

@nileio
Copy link

nileio commented Apr 10, 2020

I have the same issue and from what I found from the code - the issued npm remove command for some reason does not remove the packages but it does update package.json if those modules are found in there. also package.json file does not include the dependancies on the first install of uibuilder .. running npm remove from the command line does nothing too in that case.. I "think" the first install of the modules is the root cause of the discrepancy but just initial thoughts until further debugging. It appears that installing any other modules and removing it have no issues.. so this is why I think it has to do with the install of those dependancy modules when uibuilder is installed.
@TotallyInformation thanks for the node. Could it be because you include both vue and bootstrap-vue as deps in the uibuilder node itself ? and so npm is refusing to remove because it sees that another module being "uibuilder" itself depends on it. wonder if you take out those module dependancies and allow the user to add them manually from uibuilder node if they need to will then fix the issue because they become independant from uibuilder node itself ?

@nileio
Copy link

nileio commented Apr 10, 2020

@TotallyInformation i confirmed the fix for this issue! The fix is to remove vue & bootstrap-vue dependancies from "uibuilder" node module package.json itself. I did that and it it is now working as expected and you can add/remove packages from the editor !! :) Might be possible to move those to devDependancies instead if you needed them during dev, but worth testing that at your end :)
Now the user can install whatever he likes 👍
also worth noting that bootstrap-vue depends on bootstrap and that dependancy needs to be installed first.. The node might report that is it one of the installed packages but user should install the dependancies as needed by the front end package (in this case re-installing bootstrap from the node was required in my case) .. cheers

@T0T4R4
Copy link

T0T4R4 commented Apr 12, 2020

Good work @nileio , thanks for the update

@TotallyInformation
Copy link
Owner

@nileio, many thanks for spending time looking at this. I've been tied up with COVID related work recently but that is hopefully calming down now.

In fact, having VueJS and bootstrap-vue as dependencies does work for the core requirement - having them available for users out of the box. Bootstrap is a dependency in bootstrap-vue and so is installed automatically.

However, you've spotted the nub of the problem I think. That they are dependencies of uibuilder itself and therefore npm remove doesn't actually remove them.

I think the "fix" might be to trigger an installation of the packages post installation of uibuilder rather than having them as dependencies. That way, they will be added to the package.json file in your userDir folder instead of being in uibuilder's file. Then remove should work as expected.

This shouldn't be too hard to do with a postinstall npm script. However, you would probably have to remove and reinstall uibuilder to pick up the change.

Thoughts?

@nileio
Copy link

nileio commented May 7, 2020

@TotallyInformation thanks for response .. I think what you suggest as a postinstall script is a good solution. how do you see it working ? do you see this script running manually by the user? it might just be a matter of adding the info to run the script in the help section of the node , noting that if users like to try out the bundled vuejs/bootstrap-vue template to do that as an optional step ..

@TotallyInformation
Copy link
Owner

Hi @nileio, thanks for your response.

The script would run automatically, this is a feature of npm.

Anything less would be a backward step from the current features as there are many people who would use uibuilder to get started on building their own data-driven UI.

So uibuilder should "just work" out of the box. Just as it does today. Don't forget that this is really only fixing a problem that few people ever hit or worry about.

The bundling of bootstrap-vue is done for the same reason. It makes it trivially easy to create a reasonable looking data-driven UI with minimal effort.

@nileio
Copy link

nileio commented May 9, 2020

@TotallyInformation in this case postinstall script might be the solution and being backward compatible too is great..
i think as is the case now separating the templates from the core uibuilder code (that is running a socketio server and wrapping the msg object) is a good idea and might be an area to enhance for future releases , such as including other famous framework templates in the package..
Thanks for working on this and for this useful node.. will keep an eye on the new update..

@TotallyInformation
Copy link
Owner

separating the templates from the core uibuilder code (that is running a socketio server and wrapping the msg object)

Can you expand on that, I don't quite understand I'm afraid. There is already a Socket.IO server.

including other famous framework templates

Yes, I have started to prepare for that (the current default template code is in a VueJS sub-folder) but it is pretty low down the list of things to do.

@TotallyInformation TotallyInformation added this to the Security milestone Jun 13, 2020
@TotallyInformation
Copy link
Owner

Code for this change is now pushed to the security build. Please do try it out.

Note that, at present, I have not added any clever code to remove the old installations of vue and bootstrap-vue. If you want to get them into the right place, shut down Node-RED, manually remove and re-add uibuilder from the security branch.

Note that you don't need to do anything unless you want to be able to remove vue and bootstrap-vue.

@TotallyInformation TotallyInformation added Fixed Phew, think I got away with it. and removed Investigating What the ... !? bug It's life Jim but not as we know it! labels Jun 13, 2020
@nileio
Copy link

nileio commented Jul 14, 2020

my apology for late response been away from tinkering with nodered for a bit..

separating the templates from the core uibuilder code (that is running a socketio server and wrapping the msg object)

Can you expand on that, I don't quite understand I'm afraid. There is already a Socket.IO server.
I think I only meant to suggest that you adopt a design where there is a separation between the frontend frameworks and the core uibuilder utility code which I believe is simply running a socketio server,and then making the the msg object available. Anyway I feel this is actually the way it is designed at the moment so nothing to do there i think :)

Thanks for working on this issue. Is this now merged to master ? I recently updated to latest uibuilder version.

@TotallyInformation
Copy link
Owner

No, sorry, it is quite a major change to the way that uibuilder works for users and therefore will be included in the next full release (which is in the security branch as indicated).

@github-actions
Copy link

Stale issue message

@TotallyInformation
Copy link
Owner

The fix for this is part of uibuilder v3 which should be published in the next few weeks. Closing this now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed Phew, think I got away with it. no-issue-activity Workaround Available Yup, probably a bug but there is a temporary workaround
Projects
None yet
Development

No branches or pull requests

4 participants