NativeScript / nativescript-cli

Command-line interface for building NativeScript apps
https://www.npmjs.com/package/nativescript
Apache License 2.0
1.04k stars 195 forks source link

Place for project-persistent hooks #5576

Closed rigor789 closed 3 years ago

rigor789 commented 3 years ago

Trying to do something similar I ended using hooks. I feel the use of hooks should be easier and could achieve this issue as well. Since we all are developers, a programmatic hook to make any changes on the project is flexible and powerful (not so clean, though).

But I feel these issues:

Should this be a separate issue?

Originally posted by @xiromoreira in https://github.com/NativeScript/NativeScript/issues/9520#issuecomment-924712611

jcassidyav commented 3 years ago

This would be great, the way I do this today is to have a folder containing the hook in the project, which has a package structure writted in js and then reference it in the package.json of the project with file:// works well for the very simple hooks I need.

rigor789 commented 3 years ago

My take on this is that plugin hooks should not be handled inside the hooks folder. The original thought I believe was to allow plugins to add hooks, but let the user essentially control them: allow deleting them, disabling them etc. However, since hooks are generally installed by a postinstall script, there never was an easy way to opt-out of a hook from any given plugin/dependency.

We started ignoring the hooks folder, as well as auto-cleaning it with ns clean just because hooks are generally considered part of a 3rd party dependency, and if that dependency changes, there's a chance a hook may get removed from the dependency and then cause issues when the CLI is not able to find the hooks referenced by the scripts inside the hooks folder.

In some cases though as you said, it is useful to write custom hooks for a given project, and right now there's no easy/clean way to do so without making a plugin (could be a local folder referenced in the package.json, but still hacky).

Ideally, the hooks folder should be checked-in the repo, and not cleaned by ns clean, and for that to happen their inner working should shift to staying in node_modules and no postinstall scripts. The remaining question is, how to allow apps to opt-out of a hook from any given plugin?

jcassidyav commented 3 years ago

Given what is there today, the code which actually executes the plugins could be configured to not run particular ones e.g. for todays implementation (hooks dir), config could be added to the nativescript.config.ts

 hooks: {
    exclude: [
      {
        event: "after-prepare",
        hooks: [ "nativescript-unit-test-runner", "nativescript-firebase" ]
      }
    ]
  }

or if in a new format executed from node_modules

  hooks: {
    exclude: [
      {

        plugin: [ "@nativescript/unit-test-runner"],
        events: ["after-prepare"]
      },
      {

        plugin: [ "@nativescript/firebase"],
        events: ["after-prepare"]
      },
    ]
  }
rigor789 commented 3 years ago

My main concern is that technically hooks can "come and go" from plugins, so long-running projects could end up with a bunch of unused/stale configuration.

Just speculatively, but what if the CLI would keep track of the plugins, and their hooks automatically - ie. when a plugin adds a hook, it's added to the config and enabled by default, but once you change it, it stays disabled (won't override). If a plugin removes a hook, the CLI would remove it from the config as well, since it no longer applies anyways.

plugins: {
  '@nativescript/unit-test-runner': {
    hooks: {
      'after-prepare': false // disabled hook
    }
  }
  '@nativescript/firebase': {
    hooks: {
      'after-prepare': true // true by default, user can set to false to disable
    }
  }
}

This feels a bit verbose, and I don't like the true/false value there, but that's a minor detail we can play around with.

jcassidyav commented 3 years ago

To me that feels like a return to the webpack.config.ts problem where something is modifying a file under my source control just because I installed it or a new version of it, personally I'd prefer these files to be as minimalist as possible with only explicit deviance from the default that I put in.

I seems to me that wanting to ignore a hook added by a plugin would be an edge case? i.e. the plugin must need the hook for something in order to work properly.

In addition, you could have configuration that is like this to enable project hooks. i.e. a separate discovery mechanism for project hooks.

 hooks: {
    include: [
      {
        event: "after-prepare",
        hooks: [ "./mypathtomyhook/some-hook-i-wrote"]
      }
    ]
  }
rigor789 commented 3 years ago

Right, it is an edge case, and I agree - the CLI should not modify code under source control (the config).

What if we simplified it further to just

disabledHooks: [
  '@nativescript/firebase:after-prepare`
]

or

disabledHooks: {
  `@nativescript/firebase`: ['after-prepare'] // or 'after-prepare' without array, or 'all' or even `*`
}

I would also explore simplifying the hooks folder to perhaps a flatter structure, though how that would work is not clear yet.

For custom discovery like you mention, I would go with a similar, simplified syntax

hooks: {
  './path/to/some/hook': [ 'after-prepare' ] // or 'after-prepare' without array
}

The ergonomics would need some more thought, like would it be better to have this schema?

{
  hooks: {
    disabled: { 
      `@nativescript/firebase`: ['after-prepare'] // or 'after-prepare' without array, or 'all' or even `*`
    },
    include: {
       './path/to/some/hook': [ 'after-prepare' ] // or 'after-prepare' without array
    }
  }
}
jcassidyav commented 3 years ago

I guess the config would reflect the scope of the change.

If there is going to be a move away from the hooks directory then that looks good to me,

I presume that would involve changing @nativescript/hooks to record somewhere the hooks the plugin wants to install and where they are, and then changing the cli to look for the actual hooks in node_modules somewhere.

But the cli would have to support the old hooks directory mechanism, to remain compatible with older versions ( plus some plugins seem to write hooks outside the @nativescript/plugin mechanism e.g. the firebase plugin ).

A simpler change would be to keep the existing hooks functionality and just change the cli to ignore/execute based on the config.

Performing the filtering at runtime in that case, you do not know which plugin installed the hook, all you have is the name of the hook file ( which can be anything if a plugin has written it there)

so that scenario is covered by the likes of this

 hooks: {
    exclude: [
      {
        event: "after-prepare",
        hooks: [ "nativescript-unit-test-runner", "firebase-build-gradle" ]
      }
    ],
   include: [
      {
        event: "after-prepare",
        hooks: [ "./mypathtomyhook/some-hook-i-wrote"]
      }
    ]
  }

I like the more verbose configuration because it is self describing, which is of course purely subjective.

And you could argue that manually putting files in hooks folder is unsupported rendering some of the above moot.