Isolate scope

[NicoBurno]

Why this directive do not use isolate scope instead of ng-controller? If you need help with this - then I can help.

There is also a non-critical flaw: there are best practice - to return module.name to export default

[dennybiasiolli]

Hi @NicoBurno, thanks for opening this issue. I know @endorama is working on isolating directive's scope, but any help will be appreciated.

@endorama can you please push your WIP as a PR?

[NicoBurno]

@dennybiasiolli thank you for your prompt response!

We forked the project and today we will make appropriate changes. I will share code in this Issue

[NicoBurno]

@dennybiasiolli In our code, many improvements have been made, mainly relating to the optimization... Backward compatibility and code-style are preserved.

Is it more convenient for you to get several ordered PRs or one big PR in several commits?

[dennybiasiolli]

@NicoBurno thanks again, one PR-per-improvement would be great, so we can review faster 😉

[NicoBurno]

@dennybiasiolli OK. Until tomorrow we will put everything in order :thumbsup:

[NicoBurno]

Sorry, I hurried to conclusions about backwards compatibility.

There were several ways to ensure backward compatibility with ngController, but they all have significant drawbacks:

We can use a variable for scope in the link-function.

Example:

{
  scope: {
    pdfUrl: '<'
  },
  require: '?^ngController',
  link(iScope, element, attrs, ngCtrl) {
    let scope = iScope;

    if (ngCtrl && !attrs.pdfUrl) {
      // ngController annotate detected
      scope = scope.$parent;
    }
  }
}

But this does not solve the problem of bindings in a template (they break when you switch to the isolated scope)

We can use the getter to get variables from scope

Example:

const getScopeField = (scope, field) => {
  return scope[field] || scope.$parent[field];
}

{
  scope: {
    pdfUrl: '<'
  },
  require: '?^ngController',
  link(scope, element, attrs, ngCtrl) {
    if (ngCtrl && !attrs.pdfUrl) {
      // ngController annotate detected
      scope.pdfUrl = getScopeField(scope, 'pdfUrl');
    }
  }
}

But that's just in case we do not need an update

Also there is a variant with scope.$parent.$watch ...

... But this is already dark magic

[dennybiasiolli]

There is also the idea of converting the directive to an angular Component, isolating the scope and moving the template part outside the component. With this, we can interact with the component with events allowing users to customize their templates outside the component. We will lost backward compatibility, but we can release a major version of ngPdf with breaking changes. What do you think about this?

[NicoBurno]

@dennybiasiolli angular-component is just a angular-directive with a limited API. In the case of our library, it makes no sense to make the component. All that the component can do - you can make a directive.

If you refuse backward compatibility when going to an isolated area, then there will be no problems with the directive.

Should I do the PR without backward compatibility? If so, which branch?

[dennybiasiolli]

Ok @NicoBurno, you can make the PR starting from branch master, thanks!

[endorama]

Hello @dennybiasiolli @NicoBurno, sorry for the late replay! Do you still need my WIP or @NicoBurno implementation is already better than mine? :) You can check out mine here.

Granted, my work was just a quick WIP from a couple hours of hacking around the refactor!

Let me know!

[dennybiasiolli]

Thanks @endorama, please submit your WIP so we can take a look, thanks! 😄

[NicoBurno]

@endorama thanks for the answer!

I reviewed your implementation. This implementation is much better than the previous one =) Perhaps we could use this implementation in our product...

But, if we want to adhere to the best practices of angular-development, then we should abandon ngPdfFactory - it unnecessarily complicates the API. The best solution would be to bind configuration to directive's props.

@dennybiasiolli, @endorama I would suggest waiting until the end of the week, when I can return to work with ngPdf. And after that, you decide which implementation to take as the basis for the future major-version.