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

Move to vite for bundling #830

Open
wants to merge 2 commits into
base: chore/bump-libraries
Choose a base branch
from
Open

Conversation

juliushaertl
Copy link
Member

@juliushaertl juliushaertl commented Feb 8, 2024

@juliushaertl juliushaertl changed the base branch from main to renovate/nextcloud-vue-8.x February 8, 2024 16:08
@enjeck
Copy link
Contributor

enjeck commented Feb 9, 2024

@juliushaertl You say "based on #647", but I don't see how?

@juliushaertl
Copy link
Member Author

I based my branch locally on top of yours and then created a pull request towards that one. You can also sett that in the github ui:

Screenshot 2024-02-09 at 09 22 56

Once your PR/branch gets merged github automatically changes this, but that way I can make use of your changes already during development.

@enjeck
Copy link
Contributor

enjeck commented Feb 9, 2024

Doesn't work for me after running npm ci and then npm run dev. Am I missing something?

@juliushaertl
Copy link
Member Author

Sorry, it isn't ready yet, didn't wanted to ask for review but the ones got autoassigned by github 🙈 I'll ping you once I'm ready :)

@enjeck enjeck force-pushed the renovate/nextcloud-vue-8.x branch 2 times, most recently from 361c96e to 41969f4 Compare February 12, 2024 05:59
@blizzz blizzz added the 2. developing Work in progress label Feb 12, 2024
Base automatically changed from renovate/nextcloud-vue-8.x to main February 12, 2024 14:12
@juliushaertl juliushaertl changed the title enh/vite Move to vite for bundling Feb 12, 2024
Copy link
Contributor

@max-nextcloud max-nextcloud left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for looking into this.

I have some minor comments only.

Locally I get a warming when building:

grafik

I think the ?? '' can just be removed in this case - but it would also be nice to catch such warnings in the ci or so. However i was not able to trigger that check on its own.

I also noticed that tables have dark favicons for me now:
grafik
I don't know if that's related. Will try and build with main branch to compare.

.github/workflows/cypress.yml Outdated Show resolved Hide resolved
@max-nextcloud
Copy link
Contributor

I also noticed that tables have dark favicons for me now:

Just tried with current main with the same result. So not related to this PR.

@juliushaertl
Copy link
Member Author

Likely caused by #712 then

@juliushaertl
Copy link
Member Author

We need to decide when to merge this, currently the vite build requires nextcloud/server#36057 which is 27 only.

@juliushaertl
Copy link
Member Author

@susnux I'm having some trouble here with the npm run watch command which always exits after the first build. Is that something you've seen already? Any clue why that might happen? We just call vite --mode development build --watch

@susnux
Copy link

susnux commented Mar 12, 2024

@susnux I'm having some trouble here with the npm run watch command which always exits after the first build.

Should be fixed now: vitejs/vite#15951

@susnux
Copy link

susnux commented Jun 16, 2024

I think the ?? '' can just be removed in this case - but it would also be nice to catch such warnings in the ci or so. However i was not able to trigger that check on its own.

The warning means that the right part is never null / undefined because 'string' + variable ?? 'fallback' reads as:

  1. 'string' + variable
  2. ?? Then check the result is null or undefined
  3. Then fallback to 'fallback'.

So just brackets are missing like 'string' + (variable ?? 'fallback').

(Otherwise you might get 'stringnull' as the result)

@juliushaertl
Copy link
Member Author

juliushaertl commented Jun 18, 2024

@susnux The two issues i mentioned in person now on this PR after updating @nextcloud/dialogs

  • The file picker dialog loading fails (a quick patch for the dialogs library is below but I'm unsure if that is the proper approach)
  • Once that is patched the dialog does not return any selected file
diff --git a/lib/composables/isPublic.ts b/lib/composables/isPublic.ts
index ec89e8b..a1000ca 100644
--- a/lib/composables/isPublic.ts
+++ b/lib/composables/isPublic.ts
@@ -10,7 +10,7 @@ import { onBeforeMount, ref } from "vue"
 export const useIsPublic = () => {
        const checkIsPublic = () => (document.getElementById('isPublic') as HTMLInputElement|null)?.value === '1'

-       const isPublic = ref(true)
+       const isPublic = ref(checkIsPublic())
        onBeforeMount(() => { isPublic.value = checkIsPublic() })

        return {

@juliushaertl juliushaertl changed the base branch from main to chore/bump-libraries July 8, 2024 14:25
Signed-off-by: Julius Härtl <jus@bitgrid.net>

fix: Imports

Signed-off-by: Julius Härtl <jus@bitgrid.net>

chore: Move to new file picker api for esm compatibility

Signed-off-by: Julius Härtl <jus@bitgrid.net>

fix: Make CI pass

Signed-off-by: Julius Härtl <jus@bitgrid.net>

fix: Update file picker usage

Signed-off-by: Julius Härtl <jus@bitgrid.net>

ci: Fix cypress

Signed-off-by: Julius Härtl <jus@bitgrid.net>

fix file picker

Signed-off-by: Julius Härtl <jus@bitgrid.net>

fix cypress

Signed-off-by: Julius Härtl <jus@bitgrid.net>

fix: Proper css for all entrypoints

Signed-off-by: Julius Härtl <jus@bitgrid.net>

fix: Properly load styles

Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants