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

Switch from webpack to rollup #4584

Merged
merged 22 commits into from
Feb 8, 2021
Merged

Switch from webpack to rollup #4584

merged 22 commits into from
Feb 8, 2021

Conversation

fschade
Copy link
Contributor

@fschade fschade commented Jan 4, 2021

Motivation of change:

before we used webPack to bundle the web repository, over time other ocis addons switched to rollup wich speeds up the whole development proccess and thanks to its tree-shaking makes whole bundle smaller. We also started to restructure the repository to make it easier to understand and get started.

Changes:

  • replace webpack with rollup
  • remove unused dependencies
  • introduce yarn workspaces (monorepo)
  • cleanup scripts in package.json

Open tasks:

  • review makefiles
  • adjust docs
  • adjust drone
  • cleanup rollup config
  • resolve conflicts
  • request review

@update-docs
Copy link

update-docs bot commented Jan 4, 2021

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

Some questions / things I noticed:

  • when running build:w, a change in one workspace results in recompiling all workspaces. possible to limit this to the changed workspace only?
  • for running with oc10 we need to have a dev server - ideally not persisting to disk but to RAM, to save SSD cell max write counts.

Things I noticed about the scripts in package.json:

  • afaict scripts with ldap (docker:ldap and test:acceptance:ldap) are not needed. I added a commit that removes those.
  • would make sense to keep the script for killing dockerized selenium and redis in docker. I added a commit that adds those.
  • it should not be in scope of this repo to take care of running an ocis instance, so IMO it's good that you removed the related scripts.

Other questions:

  • Why is everything a devDependency? I can't see real dependencies anywhere. Is it specific to the workspaces feature that all dependencies are devDependencies?

@fschade
Copy link
Contributor Author

fschade commented Jan 9, 2021

Some questions / things I noticed:

  • when running build:w, a change in one workspace results in recompiling all workspaces. possible to limit this to the changed workspace only?
  • for running with oc10 we need to have a dev server - ideally not persisting to disk but to RAM, to save SSD cell max write counts.

Things I noticed about the scripts in package.json:

  • afaict scripts with ldap (docker:ldap and test:acceptance:ldap) are not needed. I added a commit that removes those.
  • would make sense to keep the script for killing dockerized selenium and redis in docker. I added a commit that adds those.
  • it should not be in scope of this repo to take care of running an ocis instance, so IMO it's good that you removed the related scripts.

Other questions:

  • Why is everything a devDependency? I can't see real dependencies anywhere. Is it specific to the workspaces feature that all dependencies are devDependencies?

Thanks for the input, true it only should rebuild the required parts.

This is because the vendor is required by all other apps and require js needs to import the updated vendor bundle in every app. I have a look if this can be fixed.

I will bring back the dev server, thanks for the input :)

maybe something like nollup to get hmr and dev sever in development. 🧐

everything is a devDependency because we do not have a real runtime dependency that is required by a index.js served by node. Instead we bundle everything to dist and in this stage there is no requirement to serve anything from node_modules

Npm mentions this here https://docs.npmjs.com/specifying-dependencies-and-devdependencies-in-a-package-json-file

"dependencies": Packages required by your application in production.
"devDependencies": Packages that are only needed for local development and testing.

@individual-it
Copy link
Member

it should not be in scope of this repo to take care of running an ocis instance, so IMO it's good that you removed the related scripts.
those script have been after the request to make the test runs simpler so that the web developer just can fire up the tests without having to think about how to setup ocis, testing app, core etc. see https://owncloud.github.io/clients/web/testing/#with-ocis-backend
so if we delete it, we will lose all that

@LukasHirt LukasHirt added Category:Change Change existing functionality Category:Technical Technical ehancements labels Feb 2, 2021
@LukasHirt
Copy link
Collaborator

LukasHirt commented Feb 2, 2021

Dev server is not working with my oc10 oauth2 setup. Trying on master works fine - trying to debug this.

@LukasHirt
Copy link
Collaborator

Dev server is not working with my oc10 oauth2 setup. Trying on master works fine - trying to debug this.

Found the issue... There was a newer version of oidc-client lib with which there was always a request to .well-known even though it was an oauth2 setup. For now, I'll pin the version to 1.10.1 where it still works. I'll open a ticket to update it to the latest version and see what changed there. Not scope of this PR.

@LukasHirt
Copy link
Collaborator

Hmm, thinking now about ports... we can set an arbitrary port for Web when running against oC10 so we could actually always pick port 9100 and that way wouldn't need two scripts for devServer.

@LukasHirt LukasHirt force-pushed the rollup branch 2 times, most recently from 2848acc to cb0de23 Compare February 3, 2021 16:32
@fschade fschade requested a review from kulmann February 3, 2021 16:33
@LukasHirt LukasHirt marked this pull request as ready for review February 3, 2021 16:34
Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

Really, really awesome job, I love it. There are some quirks that need fixing:

For both running on oc10 and ocis backend there is a JS error in the console:
null:1 Failed to load resource: the server responded with a status of 404 (Not Found). Happens on app load and app switch.

When running on ocis, switching to the accounts UI, the list of accounts is empty.

packages/web-runtime/l10n/Makefile is broken -> let's move it to a top level l10n folder and make it generic for all apps + runtime from packages.

oc10 marketplace app build is broken (remove signing step from makefile, see for yourself).

Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
docs/backend-ocis.md Outdated Show resolved Hide resolved
packages/web-runtime/src/components/UserMenu.vue Outdated Show resolved Hide resolved
packages/web-runtime/src/services/clientRegistration.js Outdated Show resolved Hide resolved
packages/web-runtime/src/store/config.js Outdated Show resolved Hide resolved
@LukasHirt
Copy link
Collaborator

LukasHirt commented Feb 4, 2021

When running on ocis, switching to the accounts UI, the list of accounts is empty.

This is not a problem with this PR. We're using oc-table in accounts and haven't yet adjusted it after the ODS update.

@kulmann
Copy link
Member

kulmann commented Feb 4, 2021

When running on ocis, switching to the accounts UI, the list of accounts is empty.

This is not a problem with this PR. We're using oc-table in accounts and haven't yet adjusted it after the ODS update.

Ah yes, right :-)

@fschade
Copy link
Contributor Author

fschade commented Feb 4, 2021

When running on ocis, switching to the accounts UI, the list of accounts is empty.

This is not a problem with this PR. We're using oc-table in accounts and haven't yet adjusted it after the ODS update.

Ah yes, right :-)

OCIS PR is in progress

Edit: owncloud/ocis#1597, we need to keep care of it in ocis.

@LukasHirt
Copy link
Collaborator

LukasHirt commented Feb 4, 2021

For both running on oc10 and ocis backend there is a JS error in the console:
null:1 Failed to load resource: the server responded with a status of 404 (Not Found). Happens on app load and app switch.

Coming from oc-icon. For some strange reason it's trying to load the icon from URL even though only name is passed as a prop. That's why it's null since we don't have the URL. Needs to be fixed in ODS most probably since I'm getting this error also on master and the demo instance.

@kulmann AFAIK we wanted to remove the URL part from the icon anyway, didn't we?

@fschade fschade changed the title [WIP] rollup Switch from webpack to rollup Feb 4, 2021
fschade and others added 7 commits February 5, 2021 16:03
cleanup dependencies
cleanup structure
optimize generated bundles
clean dependencies for every app
clean scripts
cleanup packages.json, names, version, descriptions...
add dev server for ocis and oc10
move oc10 php files and appinfo to own package
This was referenced Feb 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category:Change Change existing functionality Category:Technical Technical ehancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants