Skip to content

Conversation

@tucker-m
Copy link
Contributor

I'm opening this PR to explore ways to gradually migrate the front end from AngularJS to Vue, rather than doing a rewrite all at once. This would allow commits to be merged in more frequently, avoiding a very long-running feature branch. It would also allow front end fixes and changes to happen while converting those parts into Vue, instead of leaving the current front end untouched while the rewrite takes place.

A rough migration plan could look something like this:

  1. Start replacing the templates inside of an Angular controller's scope with Vue components.
  2. Once everything in that controller's scope is using Vue components, replace the Angular controller with plain Javascript. (Also, figure out if we should start using Vuex at this point, or later.)
  3. Repeat this until all Angular controllers and resource objects are plain Javascript objects.
  4. Replace the Angular router with the Vue router.
  5. Start using Webpack (and modules) instead of Gulp.

The application would be usable at any one of those points. The front end would not be in a broken or incomplete state while those things are being worked on.

The initial commit in this branch creates a Vue component that reads a property from an Angular controller and displays that on a page. I don't have the Vue template precompiler working right now, so that's why the component is using that render (h) format instead of the more HTML-like format. But it looks like it's not difficult to have Gulp run the Vue precompiler. I'll do that next.

There is a lot to explore here in order to make a solid plan, but I'm staring to think that migrating slowly from Angular to Vue is possible, and would be a better option than doing a rewrite that has to be merged all at once.

Next things I'm going to explore:

  • Vue precompiler for nicer-looking components
  • Interactive Vue components that call a method in a controller and trigger an API call

A Vue component, SomeComponent, reads the oldestFirst
value from the ContentController, and prints it out
on the page. This value is updated automatically when you
check/uncheck the "Reverse ordering" box in the settings menu.
@SMillerDev
Copy link
Contributor

I like this idea, and it also allows some vue work to be done side-by-side, so that is always welcome.

@tucker-m
Copy link
Contributor Author

Update: when you run the Vue template compiler, it turns your Vue components into modules. So that doesn't align with this repo's current process of just concatenating all of the Javascript into one file. I've worked around that by using Webpack to import the Vue component modules into one JS file, and then having that JS file be concatenated into the big, final JS file.

This may not be a bad thing. This would mean that we could start using Webpack in parallel with the other steps, and gradually put new code into modules, rather than doing that after all of the other steps like I originally thought.

I still need to see how this works with Nextcloud's Vue component library. I'll do that next.

@tucker-m
Copy link
Contributor Author

Nextcloud's Vue components work. You can do things like this now, where this.oldestFirst is a value from an Angular controller.

<template>
  <div>
    <p>oldest first is {{this.oldestFirst.toString()}}</p>
    <Actions>
      <ActionButton icon="icon-delete">Delete something</ActionButton>
      <ActionButton icon="icon-add">Add Something</ActionButton>
    </Actions>
  </div>
</template>

<script>
import Actions from '@nextcloud/vue/dist/Components/Actions';
import ActionButton from '@nextcloud/vue/dist/Components/ActionButton';
export default {
  props: {
    oldestFirst: Boolean
  },
  components: {
    Actions,
    ActionButton,
  }
};
</script>

@SMillerDev
Copy link
Contributor

How can I help get this out of draft state/merged?

@tucker-m
Copy link
Contributor Author

Thanks for asking! I had to leave it for a while, but I should be able to take another look at this over the next few days.

I was hoping to have a pretty solid idea of what the transition would look like before un-drafting this. Hopefully that will help avoid some "ok, how do we do this" moments in the future, although I'm sure there will be some of those anyway.

The previous commit replaced a component that made an API call, and didn't have any problems. The only thing I discovered there is that you'll need to pass in the controller and current feed item (or folder, or whatever the current "item" is) as separate attributes.

My last remaining exploration steps are to:

  1. Replace a component that uses an Angular loop with a Vue loop. (ng-repeat -> v-for).
  2. Figure out what tests for these components will look like.
  3. Measure page load times when both Angular and Vue are in the JS bundle.

If anyone wants to try those themself, that would be great. I'll try to spend some time on it tomorrow. I'll gladly accept a PR to this PR. (Although I might end up getting rid of this branch and making another to have a cleaner commit history if the maintainers agree that we want to take this approach. I've still been treating this as an experiment branch.)

tucker-m added 2 commits May 30, 2021 22:58
Added a replacement for the Explore page that uses vue-for
and vue-if.
@tucker-m
Copy link
Contributor Author

tucker-m commented Jun 5, 2021

Alright, I've done the parts that I thought might be difficult when migrating from AngularJS to Vue. I think that migrating gradually, rather than doing a big rewrite, is a good idea. All of the things I was concerned about at first can be worked around without much difficulty.

I rewrote an AngularJS component that uses ng-repeat and ng-show, replacing them with vue-for and vue-if. This turns out to be a straightforward 1-to-1 translation.

The only thing that wasn't very clean was using Jest for tests. You can use Jest to write tests for a Vue component, but because of the way this repo is set up, you need to compile the test code first as part of the webpack build. (This is not what you would usually have to do.) It also means you can't run just one test file, like npm run jest path/to/file.js, because the tests need to be part of a Javascript bundle. That makes the developer experience less than perfect, but I think it's not too bad. There might be a way to avoid it, too. I'll keep looking.

@Grotax, do you have an opinion on this? Merging this in would interfere with what @JonathanTreffler has been doing, unfortunately.

*EDIT: I should be clear that this branch should not actually be merged in. By "merging this in" I mean "doing things this way, using ng-vue to use Angular and Vue simultaneously while the front-end is converted piece by piece." I'll create a cleaner PR if the maintainers agree that this is a good approach.

@JonathanTreffler
Copy link

Well you have essentially done the same as i have in mine (like setting up webpack to handle vue).

What exactly is now replaced in vue ? Or is this the groundwork for replacing components after this is merged ?

I wouldn't especially like the workflow of creating a pull request for your fork, but rather directly to this repository for the gradual component replacement.

I am a bit worried about the bundle size, but i guess we can't really do anything about that.

@jancborchardt and @powerpaul17 had ideas about the new design and i think, that even in this gradual approach we should use that.

We definitely need vuex, but parallel with angular that seems very messy.

I am not really interested in the part that makes vue and angular work together, so i would appreciate if you (@tucker-m)could do all of that stuff 😂

In 2 weeks i will have moths of freetime and i am looking forward to working with you all on this 👍

There are some things we need to discuss:

  • Wich components should be migrated first ?
  • How do we determine the code with the angular and vue approach is ready to ship to stable ? Surely it is quite a bit easier to miss a bug in a more complicated setup ?
  • How do be bringt my changes (its not that much visually, but quite a bit in the background) into this ? (by rebasing by PR to master after this is merged and removing the work already done in this PR like webpack in mine or a new PR to master that copies the relevant additions from my PR)

@tucker-m
Copy link
Contributor Author

tucker-m commented Jun 6, 2021

Thanks for the feedback @JonathanTreffler!

What exactly is now replaced in vue ? Or is this the groundwork for replacing components after this is merged ?

This is the groundwork. My intent with this branch was to figure out how we could do it in small sections and what packages would be needed to do that. (ng-vue is the main thing needed.) No real components are replaced in this PR. When I clean up this PR, I'll replace a component as a demonstration of how future contributors should do it.

I wouldn't especially like the workflow of creating a pull request for your fork, but rather directly to this repository for the gradual component replacement.

You wouldn't need to create a pull request for my fork, the fork could be merged in and then all other Vue-conversion pull requests would go directly to the main repository, too. The strategy in this PR allows us to avoid a long-running branch.

I am a bit worried about the bundle size, but i guess we can't really do anything about that.

That does get larger, unfortunately. Here's the comparison:

Packages Size on disk Transferred (after compression)
With Angular, Vue, and Nextcloud Vue component library 3.4 MB 950 KB
Just Angular 2.3 MB 640 KB

I am not really interested in the part that makes vue and angular work together, so i would appreciate if you (@tucker-m)could do all of that stuff 😂

There wouldn't be anything to be done there, besides the things included in this PR. (Or, the cleaned-up version of this PR that I'll make.)

There are some things we need to discuss:

* Wich components should be migrated first ?

I'm not sure. In general, we would be working from the inside of the document outwards. Smaller, nested elements first, then their parent elements, until the root container is using Vue.

* How do we determine the code with the angular and vue approach is ready to ship to stable ? Surely it is quite a bit easier to miss a bug in a more complicated setup ?

Although this setup is more complicated, my thought is that we will avoid more bugs this way by being able to keep using the existing Angular controllers. That's where the real "meat" of the application is. If we still use that, and only change the templates from Angular to Vue, we won't really be changing any application logic. And that's where the bugs are.

* How do be bringt my changes (its not that much visually, but quite a bit in the background) into this ? (by rebasing by PR to master after this is merged and removing the work already done in this PR like webpack in mine or a new PR to master that copies the relevant additions from my PR)

There might be a way to apply some of what you've done on top of this PR. But your PR removes Angular and build tools like Grunt, which will need to stay until all Angular code has been removed.

@JonathanTreffler
Copy link

Thanks for the answers 👍.
Sounds great

@SMillerDev
Copy link
Contributor

I'd be fine just merging what we have and gradually replacing everything. Maybe we merge this before 16.x already?

As for storage, that doesn't cost too much nowadays so 1mb extra is negligible.

@JonathanTreffler
Copy link

JonathanTreffler commented Jun 7, 2021

As for storage, that doesn't cost too much nowadays so 1mb extra is negligible.

Well, not on a bad mobile connection, but we can't do anything about it anyways.
JS bundle sizes are quite big on most Nextcloud Apps.

@SMillerDev
Copy link
Contributor

Well, not on a bad mobile connection, but we can't do anything about it anyways.
JS bundle sizes are quite big on most Nextcloud Apps.

Most images will hurt more than the app size. (and you can always use a native app on mobile)

@SMillerDev
Copy link
Contributor

Anything I can do to help here?

@tucker-m
Copy link
Contributor Author

tucker-m commented Jul 3, 2021

I'm closing this PR in favor of #1421, which doesn't have all the extra "just messing around" commits that I put in this one. My intent with this PR was to figure out if this was something that should really be done, and it sounds like it is, so #1421 is the "for real" one.

@tucker-m tucker-m closed this Jul 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants