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

Build/use vite for package #2386

Merged
merged 43 commits into from
Jun 7, 2022
Merged

Build/use vite for package #2386

merged 43 commits into from
Jun 7, 2022

Conversation

max-nextcloud
Copy link
Collaborator

@max-nextcloud max-nextcloud commented May 12, 2022

Summary

Use vite to build lib in dist folder. We can then package this lib.

Started out using rollup but turns out using vite is easier because it knows how to handle vue better.
This also provides the other benefits of vite when working on the package.

index.html embeds the ReadOnlyEditor and allows changing the content with an input field to try it out.

  • allow adding extensions to RichText extension via configuration
  • make extensions added that way overwrite the ones with the same name
  • use EditableTable in EditorWrapper
  • styles are missing in index.html
  • something is wrong with the source maps... cannot find file when trying to create breakpoints
  • need to change package.json to actually build something
  • need to try this out in collectives.

@juliusknorr
Copy link
Member

Cool, I wasn't even aware that vite also has a library build target :)

@max-nextcloud max-nextcloud force-pushed the build/use-vite-for-package branch from 6bae498 to a19fec0 Compare May 12, 2022 10:25
@max-nextcloud max-nextcloud force-pushed the build/use-vite-for-package branch 3 times, most recently from 1389763 to 47e80cb Compare May 17, 2022 09:15
@max-nextcloud

This comment was marked as resolved.

@vinicius73
Copy link
Member

import path from path

path is a native node module, some polyfills enable us to use it in browser bundles, but it is a mistake. We should avoid the usage of native node modules.

I will try a small change for it.

@vinicius73
Copy link
Member

vinicius73 commented May 17, 2022

Webpack uses path-browserify as polyfill.
Changing to path-normalize we can remove this polyfill and make our package more "free transpiling" and smaller.

Refs.:

@max-nextcloud max-nextcloud force-pushed the build/use-vite-for-package branch 2 times, most recently from e516807 to 27f328a Compare May 23, 2022 07:42
@max-nextcloud max-nextcloud marked this pull request as ready for review May 23, 2022 07:43
@max-nextcloud max-nextcloud marked this pull request as draft May 23, 2022 08:15
@max-nextcloud max-nextcloud force-pushed the build/use-vite-for-package branch 4 times, most recently from eae5dfd to 32e6afe Compare May 31, 2022 14:20
@max-nextcloud max-nextcloud marked this pull request as ready for review May 31, 2022 14:20
@max-nextcloud
Copy link
Collaborator Author

/compile

@max-nextcloud max-nextcloud requested a review from julien-nc May 31, 2022 14:23
@max-nextcloud max-nextcloud force-pushed the build/use-vite-for-package branch from 32e6afe to 7a605d0 Compare May 31, 2022 14:27
@max-nextcloud
Copy link
Collaborator Author

/compile

Copy link
Member

@vinicius73 vinicius73 left a comment

Choose a reason for hiding this comment

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

I haven't tested yet, but looks very good. Thanks!

package.json Outdated Show resolved Hide resolved
src/components/PlaintextReader.vue Outdated Show resolved Hide resolved
src/components/RichtextReader.vue Outdated Show resolved Hide resolved
@max-nextcloud max-nextcloud force-pushed the build/use-vite-for-package branch from eed60c9 to 06efee6 Compare May 31, 2022 14:37
Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

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

Naive remark:
Using npm 7.24.2 and node v14.19.3, when running npm run dev:package or npm run build:package Vite complains about not finding the icons (which are there):

[vite]: Rollup failed to resolve import "vue-material-design-icons/Loading" from "src/components/icons.js".
This is most likely unintended because it can break your application at runtime.
If you do want to externalize this module explicitly add it to
`build.rollupOptions.external`
error during build:
Error: [vite]: Rollup failed to resolve import "vue-material-design-icons/Loading" from "src/components/icons.js".
This is most likely unintended because it can break your application at runtime.
If you do want to externalize this module explicitly add it to
`build.rollupOptions.external`
    at onRollupWarning (/server25/apps/text/node_modules/vite/dist/node/chunks/dep-59dc6e00.js:41489:19)
    at onwarn (/server25/apps/text/node_modules/vite/dist/node/chunks/dep-59dc6e00.js:41305:13)
    at Object.onwarn (/server25/apps/text/node_modules/rollup/dist/shared/rollup.js:23202:13)
    at ModuleLoader.handleResolveId (/server25/apps/text/node_modules/rollup/dist/shared/rollup.js:22492:26)
    at /server25/apps/text/node_modules/rollup/dist/shared/rollup.js:22453:26

@max-nextcloud
Copy link
Collaborator Author

Using npm 7.24.2 and node v14.19.3, when running npm run dev:package or npm run build:package Vite complains about not finding the icons (which are there):

should be fixed.

Vinicius Reis and others added 16 commits June 7, 2022 19:42
Signed-off-by: Vinicius Reis <[email protected]>
Signed-off-by: Max <[email protected]>
Peer dependencies should be provided by the dependent package.
Add them to our dev dependencies.

Signed-off-by: Max <[email protected]>
Add an `EditableTable` node that has all the buttons for changing the table.
Use it in the Editor while keeping the plain `Table` in RichtextReader.

Signed-off-by: Max <[email protected]>
Only list the package dependencies as dependencies in package.json.
Other dependencies of the app are now listed in devDependencies.

Signed-off-by: Max <[email protected]>
The app name `text` in appinfo
and the package name `@nextcloud/text in package.json differ.
webpack-vue-config reads the app name from package.json.
But for bundling we need the one from appinfo.
Can be removed after merge of
nextcloud-libraries/webpack-vue-config#338
or a successor.

Signed-off-by: Max <[email protected]>
`vite dev` loads all imports of a file. So better keep them separated.
EditableTable is causing issues because @nextcloud/vue does not play nicely with vite yet.

Signed-off-by: Max <[email protected]>
`vite dev` will load all imports in imported files.
So for now it is better to not bundle multiple files in indexes.

Signed-off-by: Max <[email protected]>
Be consistent with other uses of `RichText` and `PlainText`.

Signed-off-by: Max <[email protected]>
* only import prosemirror css once.
* disconnect after receiving a 409 response in polling backend.
* clean up remains of `RichtextOptions`.
* use the `isRichEditor` prop in Reader.
* add cypress test for conflict display.

Signed-off-by: Max <[email protected]>
@max-nextcloud max-nextcloud force-pushed the build/use-vite-for-package branch from 44338f7 to b1ccb60 Compare June 7, 2022 18:24
@max-nextcloud
Copy link
Collaborator Author

/compile

Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@max-nextcloud max-nextcloud force-pushed the build/use-vite-for-package branch from bdff106 to ab03ba3 Compare June 7, 2022 20:13
@max-nextcloud
Copy link
Collaborator Author

/compile

@max-nextcloud max-nextcloud force-pushed the build/use-vite-for-package branch from 0b686de to 6acb55c Compare June 7, 2022 21:05
Tests were failing because cypress proceeded
with adding files to a folder that had not been created yet:
https://github.com/nextcloud/text/runs/6780248454?check_suite_focus=true

Do not create the callout test directory twice.
Nested cypress blocks will execute `beforeEach` on all levels.

Signed-off-by: Max <[email protected]>
@max-nextcloud max-nextcloud force-pushed the build/use-vite-for-package branch from 6acb55c to 0162bb0 Compare June 7, 2022 21:12
@max-nextcloud
Copy link
Collaborator Author

/compile

Signed-off-by: nextcloud-command <[email protected]>
@max-nextcloud max-nextcloud merged commit 2eac243 into master Jun 7, 2022
@delete-merged-branch delete-merged-branch bot deleted the build/use-vite-for-package branch June 7, 2022 21:23
@max-nextcloud max-nextcloud mentioned this pull request Feb 26, 2024
4 tasks
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.

5 participants