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

adds utility.js as facade to generic javascript utility functions #7738

Conversation

nathanwoulfe
Copy link
Contributor

Prerequisites

  • I have added steps to test this contribution in the description below

This is the first step required for the AngularJs divorce proceedings...

Description

Changes here introduce utility.js to either replace AngularJs ng-function stuff, or to provide a facade to underscore functions (which in turn replace AngularJs ng-function stuff). Ideally at some point in the future the underscore dependency can be removed too, but for now it's required to manage some of the more complex bits.

Functions from AngularJs' ng-functions not included here are either not used in the current codebase, or can be replaced with vanilla JS (things like angular.forEach, for example).

Open to changing the naming, but felt umb was an appropriate namespacing, particularly as Umbraco is already used to provide access to the server variables collection. I guess we could attach methods to that object, but that might prove confusing.

Given all that, this change doesn't actually do anything, just adds a collection of new methods. Can verify these are available globally by bashing umb into the browser console, to return the object and its contained methods. Can even test the methods if you're so inclined, most are obvious in their intent.

@nathanwoulfe
Copy link
Contributor Author

Annnd had to rework things a bit, the first way I'd added utility.js didn't play nice with Karma, so tests were failing. Not sure why, but the umb global couldn't be found.

Instead, I'm now bundling the utility file into the umbraco.services module. Tests all pass, umb utility object is available everywhere. It doesn't feel like the best solution, am open to other ways.

@nathanwoulfe nathanwoulfe changed the title adds utility.js, includes it in JsIntialize.js adds utility.js as facade to generic javascript utility functions Mar 2, 2020
@nielslyngsoe
Copy link
Member

Hi @nathanwoulfe
Good start — I do think we need to consider making the name a bit more explicit.
Not that I have a great name up my sleeve...

I don't think it needs to be named anything in relation to Umbraco, so I would say we could get rid of the "umb".
Utility is a good name but maybe too general?

Any thoughts, ideas to elaborate on?

@nathanwoulfe
Copy link
Contributor Author

@nielslyngsoe I jumped between utility and helpers and those generic type names. Definitely happy to change from umb too - was really a case of having to call it something!

Maybe utility isn't too bad... After all, that is what it is

@nielslyngsoe
Copy link
Member

nielslyngsoe commented Mar 2, 2020

@nathanwoulfe if you like it, I think we can go with it 😀. Then we can always be enlightened later if someone has a great name. 🌟

@nathanwoulfe
Copy link
Contributor Author

Descriptive naming is good naming. I'll update the PR in the morning.

@nielslyngsoe
Copy link
Member

Cool, and just let me know if you need anything or have some thoughts you like some perspectives to 😊 then I'm right here.

… Updated umb.noop examples to Utility.noop
Copy link
Contributor

@abjerner abjerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @nathanwoulfe,

Great work here 👍

I added a few notes for your changes. Generally it seems that we can't just replace the functions from Angular with similar functions from Underscore, as they differ in their implementations.

Most likely this only affects edge cases, but I believe this is something we should test before making the switch. Not being a frontender, I don't really know how to do this the best way though 🤷

src/Umbraco.Web.UI.Client/.eslintrc Outdated Show resolved Hide resolved
src/Umbraco.Web.UI.Client/gulp/config.js Outdated Show resolved Hide resolved
src/Umbraco.Web.UI.Client/src/utility.js Outdated Show resolved Hide resolved
src/Umbraco.Web.UI.Client/src/utility.js Outdated Show resolved Hide resolved
Copy link
Contributor

@abjerner abjerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nathanwoulfe thanks for the last changes. I think it looks good now, as the functions have been abstracted away in the new Utilities variable, but still with the same implementation 😄

I had a quick search through Umbraco's source code, and angular.extend seems to be used a number of times. Could you add a new function for that as well?

https://github.com/umbraco/Umbraco-CMS/search?p=1&q=angular.extend&unscoped_q=angular.extend

Umbraco also uses angular.forEach a lot, but I'd imagine this can be replaced by Array.prototype.forEach instead, so no need to add an additional function for that.

@nathanwoulfe
Copy link
Contributor Author

@abjerner, yeah, had assume Array.prototype.forEach in place of angular.forEach

Had intended using Object.assign in place of angular.extend, but that might have issues with potentially overriding hashkeys - extend protects $$props, which is required when working with two AngularJs objects... Will add it in, but can probably apply it case-by-case - depending on the context, Object.assign might be perfectly ok.

Copy link
Contributor

@abjerner abjerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nathanwoulfe awesome - I'm happy to approve this 👍

@nathanwoulfe nathanwoulfe mentioned this pull request Mar 10, 2020
17 tasks
@nul800sebastiaan nul800sebastiaan merged commit b082d40 into umbraco:v8/contrib Mar 24, 2020
@nul800sebastiaan
Copy link
Member

Hey @nathanwoulfe - great start! Sorry for the delay but world events kind of interfered 🙈

Thanks again!! 👍 ⭐️

@nul800sebastiaan nul800sebastiaan linked an issue Mar 26, 2020 that may be closed by this pull request
17 tasks
@nathanwoulfe
Copy link
Contributor Author

@nul800sebastiaan these are strange times indeed...

Having this merged means the other related tasks can now be tackled 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Start decoupling from AngularJs
4 participants