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

Move externalModules back into the user dir #3064

Merged
merged 8 commits into from
Jul 15, 2021
Merged

Conversation

knolleary
Copy link
Member

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Proposed changes

In 1.3 we introduced the ability for the Function node to specify additional modules via the UI.

For various reasons, we decided to install those extra modules under ~/.node-red/externalModules. This was whilst trying to solve various issues around the full lifecycle management of those modules.

However we never released support for removing modules via the UI - that has to be done on the command line. So none of the issues that were addressed by having a separate directory actually apply.

Feedback from the community was not overly positive on having a separate directory. Users want a single package.json for their application that covers everything - rather than having one for the app and one for the external modules.

This PR changes the behaviour to install and load external modules from the user dir - ~/.node-red.

This means they live in the top level package.json - making it easier to manage.

The code checks for the existence of the 'old' externalModules directory and displays this warning if it is found:

13 Jul 11:23:31 - [warn]

---------------------------------------------------------------------
Node-RED 1.x external modules directory detected:
    /Users/nol/.node-red/externalModules
This directory is no longer used. External Modules will be
reinstalled in your Node-RED user directory:
   /Users/nol/.node-red
Delete the old externalModules directory to stop this message.
---------------------------------------------------------------------

13 Jul 11:23:31 - [info] Starting flows

The runtime will reinstall the modules (rather than try to copy/move files) as needed - just as if they were requested for the first time.

This is a change we can only make on a major version bump. If we don't do this for 2.0, then I suspect the externalModules directory usage will get too well established before we can consider the change again in 3.0.

@knolleary knolleary added the 2.0 label Jul 13, 2021
@knolleary knolleary requested a review from dceejay July 13, 2021 10:31
@knolleary
Copy link
Member Author

@HiroyasuNishiyama I would like your feedback on this before we merge - in case there is any problems you can see with this change.

@coveralls
Copy link

coveralls commented Jul 13, 2021

Coverage Status

Coverage decreased (-0.003%) to 67.572% when pulling 5bfb012 on revert-external-modules-dir into ed5567f on dev.

@HiroyasuNishiyama
Copy link
Member

As you described, if we leave the removal of the library to the user's responsibility, it may be OK to have one directory.
However, I have concern that it will make it difficult to introduce a delete UI in the future.

By the way, when I tried this PR, I got an error in my environment.
Following line in externalModules.js makes knownExternalModules to be undefined value in some case, this causes exception in the following execution.

        knownExternalModules = pkgFile.dependencies;

@knolleary
Copy link
Member Author

Making some more changes to the Function node module handling.

  1. added RED.import that uses dynamic imports instead of require. This works for both CJS and ES6 modules (whereas RED.require only works for CJS modules).
  2. changed the Function node to use this new API - this means the Function node can now use both module types.

Given the above, it is no longer appropriate to present the module list as if it were a line of code - const variable = require("module");. So I've updated the UI to this:

image

The functionality remains the same - the module name gives you a TypedInput listing the modules already in use, plus an "Other" option. The variable name is autogenerated from the module name, but can be customised if needed.

I am still thinking about the move of the module list into the top-level package.json. I still think we should do it, but we should have a way to distinguish modules installed manually vs modules installed via the UI. I think we can do this by keeping a list of modules in runtime settings (ie .config.xxx.json) of the modules we automatically install. Will implement and try it out.

@knolleary
Copy link
Member Author

With the latest commit, we now keep a record of any modules the runtime installs in .config.modules.json. This means, in the future, we can track which modules can be deleted if the user no longer needs them in their flows.

I think this is good enough for 2.0 - and gives a way forward to have a better management api/ui for external modules in a later release.

@knolleary knolleary merged commit eb4625a into dev Jul 15, 2021
@knolleary knolleary deleted the revert-external-modules-dir branch July 15, 2021 08:56
@Steve-Mcl
Copy link
Contributor

Hi @knolleary

I tested this patch with monaco editor to ensure types are picked up for intellisense - all good :)

One very VERY minor thing (might be a chrome issue) but the new UI is a couple of pixels too tall and causes the scrollbar to appear...

yWdiAiw7Ep

Cheers.

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.

None yet

4 participants