1. Atlassian
  2. Project: Atlassian
  3. aui

Pull requests

#270 Merged
Repository
zephor
Branch
AUI-1362
Repository
atlassian
Branch
master

removing raphael from inline-dialog and replacing with a css arrow

Author
  1. Benjamin Wong
Reviewers
Description
No description
  • Issues AUI-1362
  • Learn about pull requests

Comments (20)

  1. Adam Ahmed [Atlassian]

    I love this, but it's a breaking change.

    There are at least a few people doing weird things with getArrowPath and getArrowAttributes (at least I know it's used in FECRU's Commit Graph).

    I think you should leave in the old code path, deprecated, for a few versions. Use the new CSS by default, but if either getArrowPath or getArrowAttributes is specified in the options, add a class that removes the CSS arrow and run the SVG codepath. Might be good to log to the console in the deprecated codepath so people know they're doing it wrong.

    1. Benjamin Wong author

      Ahh right, I didn't realise they were part of the API thought they were just used internally.

      I don't think we need to run the actual path anymore. getArrowPath and getArrowAttributes code path was supposed to return the information and nothing more. If there are people using that information to do hacky things based on the underlying implementation I'm not sure we should accomodate for that...

    2. Ben Buchanan

      The end goal of this work is to remove Raphael from the core AUI load, so I don't wan't to leave in anything that's going to prevent that. How about this...

      • we go ahead with this update and deprecate all the weird addons to the API
      • we create a legacy copy of inline dialog as it stands; and create a web-resource with its own dependency on raphael. this will also log errors, etc. Anyone who must keep the old version for a while can load the extra web-resource.

      Nasty but it should work. Thoughts?

      1. Adam Ahmed [Atlassian]

        That's not how deprecation works. If you force people to change their code, then it's still a breaking change. Imagine existing plugins that are installed when a product is updated to this version of AUI. Those plugins will break because they are from before this new legacy resource even existed.

        If you really support a deprecation period of 2 major versions, you have to support these methods for 2 major versions. If you don't, then don't claim you do. For what it's worth, I think you should change that policy, but that's a separate discussion.

        Your current deprecation policy means you can't remove Raphael for 2 major versions anyway so no reason you can't leave the deprecated code in InlineDialog during that period.

        1. Ben Buchanan

          I don't think we can wait that long to remove Raphael; and in fact I don't think anyone would be happy if we did. We might just have to make a breaking change here.

          We probably need to change the policy (which I think is only in a draft on EAC anyway) to make it clearer that AUI may provide continued support via a legacy web resource. I don't think we can move forward at the pace expected otherwise.

          1. Adam Ahmed [Atlassian]

            Yeah cool, shorter deprecation period makes sense to me.

            The legacy resource still doesn't, though. That just moves the breaking change from being in the UI to being in the plugin system.

            You should release the next version of AUI with support for Raphael and CSS arrows. Then in some future version (could be the next one if you want), you can remove the Raphael support. That way plugin devs have a real release they can code against to test their new code and release their plugin.

            1. Benjamin Wong author

              I'd argue that deprecation is for features and that the ability to override our API is not a feature. The fact that we use SVG to draw the arrow is also not a feature. That means if you do this, and we change the underlying implementation you SHOULD break. Deprecation is for good citizens.

              That said the getArrowAttributes and getArrowPath functions ARE features and we can easily put them in and deprecate them normally. I think we have to draw the line somewhere and that somewhere in this case is when you override our functions.

              In any case I feel like we should make a FERM decision about deprecating Raphael and then perhaps see how removing the arrow SVG affects the products (I don't think we need to worry about plugins doing those other dodgy things). If it's only FECRU let's come to an agreement for a solution.

              1. Adam Ahmed [Atlassian]

                I'd argue that deprecation is for features and that the ability to override our API is not a feature.

                Yeah I would say AJS.InlineDialog = function() { console.log('lol I do what I want')} is not something we support.

                That said the getArrowAttributes and getArrowPath functions ARE features and we can easily put them in and deprecate them normally.

                Agree, they are options that are part of the InlineDialog API. Just having the JS not break isn't enough though. If you don't take their output and use it as expected to reshape and color the arrow, you've still broken them.

                In any case I feel like we should make a FERM decision about deprecating Raphael and then perhaps see how removing the arrow SVG affects the products (I don't think we need to worry about plugins doing those other dodgy things).

                Yeah let's take it to FERM then. But we do have to worry about plugins doing "dodgy" things because our API supports them doing it. Our API says "hey you can do a dodgy thing" and it's well documented https://developer.atlassian.com/display/AUI/Inline+Dialog

                1. Ben Buchanan

                  Those methods don't seem to actually need Raphael, they seem to return the same thing no matter what. So we can leave them in, log errors, etc; and still take Raphael out.

                  From memory FERM already gave this the nod a while back actually. Will have to dig through the notes.

                  1. Adam Ahmed [Atlassian]

                    They are options that you pass in, not methods that you call. These are just the default values. If someone is going with the default values, then it's fine to ignore these and use the CSS instead.

                    If they have used the opportunity we provided and overrode these functions with their own functions, then the CSS will not work as expected.

                    I'm sure everyone gives the nod to take out Raphael. I do too. :) The only question is how you do it. You have to do it in a way that is fair to plugin devs (and product devs).

                    1. Benjamin Wong author

                      So after reading the release notes for AUI 3.5 (since the docs aren't clear at all what those functions are meant for) I now agree that we can't just remove them. Sorry I didn't realise what our API was offering. We've actually gone and said 'Hey override this function, that's part of our API', I didn't realise we'd actually done that.

                      Now we have a blocker for removing Raphael, one that can't easily be resolved because we've said what we've said. So the question is, do we go ahead and remove it anyway and label this an exception to the rule? Is the weight of Raphael that big a deal that we want to get rid of it at the expense of our policy? Or do we stay the course and deprecate it in two versions. Seems this needs further discussion in a different forum.

                      As for this PR I'll revert it totally and add in an option for a css arrow since it's the new feature.

                    2. Ben Buchanan

                      Yeah on reflection we have to slow it down and deprecate (really deprecate, not break) this in 5.1; guess the removal of Raphael will have to wait for 6.0. Nagdamnit ;)

  2. Jonathon Creenaune

    Starting this as a new thread as BB indentation gets way too narrow :)

    Agree that this is an API breakage; maybe there are other workarounds. The end goal is:

    • if someone has specified getArrowPath / getArrowAttributes, that should be respected. This implies pulling in Raphael
    • if they haven't, we shouldn't pull in Raphael

    There are ways around this. The web-resource system makes it hard, but what if we lazily pull in raphael when a client passes in getArrowPath / getArrowAttributes? There are ways to do this without adding a full-blown AMD system to AUI.

      1. Benjamin Wong author

        I think whether or not libraries are applicable to the deprecation is another issue (I'm in two minds about this) but also I think we have grounds for an exception in this particular case anyway. Bigger discussion for a bigger forum?

        Everyone seemed happy that we were going to remove Raphael, but no-one thought about this, us included. Sounds like something we should discuss.

  3. Gilmore Davidson

    Bonfire also overrides the getArrowPath option in a couple of places, but we have to support multiple versions of JIRA (and therefore older versions of AUI) with the one codebase.

    I'm not saying "don't do this change", since we can easily switch to a custom CSS method that mimics the AUI one, but just pointing out that there will definitely need to be a decent notice period.

  4. Benjamin Wong author

    Updated with changes mentioned above, the API now uses the css by default unless either a getArrowAttribute or getArrowPath function is passed into the options