Skip to content

Conversation

@gary-kim
Copy link
Member

@gary-kim gary-kim commented Feb 11, 2020

Closes #252

Being a bit conservative with the milestone for now. 😄

There's quite a lot more that could be done for cleanup but I think it'd be better to put that in a follow-up PR.

EDIT: Someone who has access to settings for the repo: could you enable the projects feature? I'd like to make a project for migration to Vue.
EDIT 3: Once this is done, PR incoming for migrating admin settings to Vue.
EDIT 4:
@eneiluj @jancborchardt package-lock.json and composer.lock being in the .gitignore file caused me some confusion. I just read #44.
I just removed those lines from .gitignore and committed those files in this PR.
Just for future reference, package-lock.json and composer.lock should be committed into source control: package-lock.json documentation && composer.lock documentation 😄. (I don't mean that with any negative intent)

@gary-kim gary-kim added this to the 0.3 📍 advanced features milestone Feb 11, 2020
@gary-kim gary-kim force-pushed the fix/252/webpack branch 11 times, most recently from b662d66 to 7fef5ba Compare February 11, 2020 12:44
@gary-kim gary-kim changed the title WIP: Use Webpack Use Webpack Feb 11, 2020
@gary-kim gary-kim force-pushed the fix/252/webpack branch 2 times, most recently from 0a6537a to 9abcee6 Compare February 11, 2020 13:04
@gary-kim gary-kim self-assigned this Feb 11, 2020
@ChristophWurst
Copy link
Member

Just for future reference, package-lock.json and composer.lock should be committed into source control: package-lock.json documentation && composer.lock documentation smile. (I don't mean that with any negative intent)

Yes!

@ChristophWurst
Copy link
Member

+7,999

Are you trying to sell the PR? :P

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Seems good in general.

Neither did I test this nor have I a clue about the code of the app 🙊

@tacruc
Copy link
Collaborator

tacruc commented Feb 11, 2020

How does this work together with #217?
As I see both PRs add some webpack.prod.js for example.
@paulschwoerer Cloud you review, too.

@gary-kim
Copy link
Member Author

gary-kim commented Feb 11, 2020

Huh, interesting. This is going to be a messy merge. #217 set up a lot of the same things as this PR with the main difference being that #217, the setup was just for the new feature and here, the idea is to simply migrate all of the JS for the app.

Considering the name of #217, I didn't think to look in it for a change like this.

@tacruc
Copy link
Collaborator

tacruc commented Feb 12, 2020

Thats what I was afraid of. What would be the besteht order of merging them?
@paulschwoerer

Copy link
Contributor

@paulschwoerer paulschwoerer left a comment

Choose a reason for hiding this comment

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

Please have a look at the @nextcloud/router dependency in package.json. Shouldn't it be a production dependency?

Apart from that LGTM

@paulschwoerer
Copy link
Contributor

paulschwoerer commented Feb 12, 2020

@gary-kim Solid work!

What would be the besteht order of merging them?

It should actually be quite straightforward to merge this with the changes I did. I'd suggest to merge this commit first and I'll take care about merging the changes into #217 before merging #217 into master.

I'd like to make a project for migration to Vue.

I was also interested in migrating the app to Vue, I'd be glad to lend a hand in doing so. In my PR you might find some useful components, that can be expanded on.

Signed-off-by: Gary Kim <gary@garykim.dev>
@gary-kim
Copy link
Member Author

@gary-kim Solid work!

You too @paulschwoerer!

In my PR you might find some useful components, that can be expanded on.

Thank you! I'll be sure to make good use of those.

@tacruc
Copy link
Collaborator

tacruc commented Feb 14, 2020

Is it ready to merge?

@gary-kim
Copy link
Member Author

gary-kim commented Feb 14, 2020

Is it ready to merge?

🚀 Ready to merge!

EDIT: Actually, let me add the node_modules to the ignore for release first 🙈
EDIT 2: Done!

Signed-off-by: Gary Kim <gary@garykim.dev>
@tacruc tacruc merged commit 27f38c1 into master Feb 14, 2020
@tacruc tacruc deleted the fix/252/webpack branch February 14, 2020 11:08
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.

Use a bundler

5 participants