cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Grieve <agri...@chromium.org>
Subject Re: Proposal: hooks support for plugins
Date Fri, 18 Jul 2014 20:25:33 GMT
Finally got to having a look. Lots of neat stuff in there! Some specific
feedback:

- Not a fan of "<script>", "<hook>" might be better
- context.commonModules is a great idea, but I don't think exposing
requireCordovaModule or elementtree is a good idea.
  - E.g. what if we want to drop our dependency on elementtree?
  - I think it's important that we only expose cordova-lib's public API to
hooks.
  - For the same reason, let's not expose cordovaUtil. just make the
top-level "cordova" object available. If there are other APIs that are
needed, we should add them to the public API.
- Since you're making a new class, please don't call it "Hooker"
("HookManager", "HookRunner", etc)
- Looks like you already added "ScriptsRunner.js"
  - Don't capitalize the file unless it exports as constructor
  - Maybe Hooker can be combined with ScriptsRunner?

Supporting node hooks and adding plugin hooks are two big changes. Didn't
look to see how the commits had things split up, but I'd like to see one
change go in separately from the other.

I'm a bit concerned that plugin hooks won't work well due to it being hard
to write x-platform, server builds like PGBuild, Telerik won't want to have
hooks run on their servers, Thym likely can't support it.
Security-wise, I'd feel better about it if we prompted the user before
enabling plugin scripts to run. Just a simple Y/N confirmation would be
enough. E.g.: "Plugin org.foo.bar contains an installation hook. Allow it
to run?"




On Fri, Jul 11, 2014 at 3:42 AM, Sergey Grebnov (Akvelon) <
v-segreb@microsoft.com> wrote:

> Updated docs[1] as per review. Looking forward to community feedback
> regarding:
> 1 . Idea of defining hook scripts via config.xml and plugin.xml
> 2. New Script Interface for .js files (not used for /hooks scripts for
> backward compatibility)
>
> [1]https://github.com/apache/cordova-lib/pull/55
> [2]
> https://github.com/MSOpenTech/cordova-lib/blob/CB-6481-hooks/cordova-lib/templates/hooks-README.md#script-interface
>
> Thx!
> Sergey
> -----Original Message-----
> From: Shazron [mailto:shazron@gmail.com]
> Sent: Wednesday, July 9, 2014 9:26 PM
> To: dev@cordova.apache.org
> Subject: Re: Proposal: hooks support for plugins
>
> Ah ok. However I think since it is effectively deprecated we should not
> advertise the old way as a supported way to do things but add another
> section saying that support for "./cordova/hooks" may be removed in the
> future, and perhaps give a timeline for removal.
>
> On Wed, Jul 9, 2014 at 10:15 AM, Josh Soref <jsoref@blackberry.com> wrote:
> > Shazron wrote:‎
> >> from the Cordova Hooks doc:
> >> "This directory used to exist at .cordova/hooks, but has now been
> >> moved to the project root."
> >> -> doesn't this imply that .cordova/hooks
> >> does not work anymore, you should use hooks/ ?‎
> >
> > I believe it's supported for backwards compatibility (just as
> > www/config.xml is) ‎
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message