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

Remove nextcloud scope from app name #338

Closed

Conversation

max-nextcloud
Copy link
Contributor

Allow publishing an app and an npm package in the @nextcloud scope
from the same source.

Allow publishing an app and an npm package in the @nextcloud scope
from the same source.

Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud max-nextcloud force-pushed the feature/strip-nextcloud-scope-from-name branch from 6dc53d0 to 07462e0 Compare May 17, 2022 10:11
@max-nextcloud
Copy link
Contributor Author

max-nextcloud commented May 17, 2022

For a bit of context:

We are working on packaging some of the text app for reuse in other apps (collectives for now, deck later). In order to ease development we want to keep the @nextcloud/text package and the text app in the same source tree.

We are considering to move to a monorepo with multiple package.json files - but for now we're building a dist folder for the package and js for inclusion in the app.

For the package we want to use the @nextcloud scope - for the app we want to avoid having an @nextcloud subdirectory in the js folder. So to be able to use the default config from this package we need to strip the @nextcloud/ prefix from the package name.

@skjnldsv
Copy link
Contributor

We are considering to move to a monorepo with multiple package.json files - but for now we're building a dist folder for the package and js for inclusion in the app.

Careful about this, in the past we looked at this multiple times, and there is always a need for third party tooling, which make the management rather complicated iirc

@artonge
Copy link
Contributor

artonge commented May 17, 2022

Not sure that I fully understand the need and motivation here. Would having an additional repo for the package be that much of a burden?

@max-nextcloud
Copy link
Contributor Author

... monorepo ...

Careful about this, in the past we looked at this multiple times, and there is always a need for third party tooling, which make the management rather complicated iirc

Yes - that's why were trying to avoid it.

@max-nextcloud
Copy link
Contributor Author

Not sure that I fully understand the need and motivation here. Would having an additional repo for the package be that much of a burden?

The short answer is 'Yes.' 😉

Let me try to also explain the long answer.

Maintenance

First of all... it's not just another repo. It comes with a fresh git history, issue tracker, changelog, release cycle, test strategy and all that. We'd have the nextcloud/text repo and then the nextcloud/nextcloud-text repo? Sounds rather confusing than helpful to me.

We currently use backportbot-nextcloud for backporting fixes to previous versions. We still expect quite a bit of churn in the given part of the codebase as we just released a bunch of new features in NC 24.

Our current approach just adds a new build target and leaves the rest of our workflows as is. Moving the package to a separate repo would imply that we rewrite each fix by hand for the existing stable branches.

Package layout

What we want to achieve for now is to reuse code between Text and Collectives for rendering markdown. We mainly want to share css and tiptap nodes to make sure they look the same. We do so by exporting a single vue component called RichTextReader that receives the markdown via a content prop and renders it the way text would.

Text itself is not just a reader but an editor so it adds @nextcloud/vue components to certain tiptap nodes. For example the TableRow is rendered in a TableRowView that adds buttons to add table rows before and after and remove the row.
So if we want to build text on top of a package we're left with two options:

Minimal Package

Basically what we are packaging now - RichTextReader. But we would also have to expose all the tiptap nodes so text can add its buttons on top of them when using them in the Editor. So we would have to expose a lot more 'internals' of the package than we currently do.

Package most of text

We could also package the text editor alongside RichTextReader. This would keep the nodes and their extended views in the same repo. We'd need a package that can do tree shaking to only include the relevant parts of the package in collectives. I have not been able to build esm with tree shaking that include @nextcloud/vue components. I'll try again and open an issue in @nextcloud/vue about that.

Why have a separate repo?

Even assuming we manage to sort out the issues with a separate repo... what would the benefit of it be? It would save us the single line change to this repo - but besides that?

Creating packages from nextcloud apps

I actually think that building a package from a nextcloud apps source is a viable approach. Besides the reuse of the name in the webpack config i don't see any conflicting settings in package.json. We did not make use of files, main, module or type yet. In fact package.json so far is just for managing dependencies. I was able to move all dependencies not needed by the package into devDependencies without any issue thus far - but i might be missing something there.

@skjnldsv
Copy link
Contributor

@max-nextcloud can you raise that issue at https://github.com/nextcloud/standards ?
I think this would benefit from collectively take a decision on this?

@max-nextcloud
Copy link
Contributor Author

@max-nextcloud can you raise that issue at https://github.com/nextcloud/standards ? I think this would benefit from collectively take a decision on this?

I opened nextcloud/standards#3 to start a discussion there.

However for now i don't necessarily want to turn this into a standard just now. I'd be happy to try it out with text for a release cycle or two and see if it works for us. This might help inform the discussion.

Copy link

@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.

Just a small idea to improve this implementation.

@@ -28,6 +28,7 @@ const NodePolyfillPlugin = require('node-polyfill-webpack-plugin')
const TerserPlugin = require('terser-webpack-plugin')

const appName = process.env.npm_package_name
.replace('@nextcloud/', '')
Copy link

Choose a reason for hiding this comment

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

Unfortunately, it can create unpredictable behaviors across all our apps ecosystem.
Instead of change it directly, we can export this object as a function (in other file maybe) and using parameters change the appName

config.js

const buildConfig = ({ appName }) => {
  return {
    // ommited
    output: {
      publicPath: path.join('/apps/', appName, '/js/'),
      filename: `${appName}-[name].js?v=[contenthash]`,
    }
  }
}

module.exports = { appName }

in webpack.config.js

const { buildConfig } = require(./config.js)

const appName = process.env.npm_package_name

module.exports = buildConfig({ appName })

We will be able to import buildConfig instead of webpack.config.js into the applications and be able to do other changes like different output paths if we want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm starting to think the appname should be taken from appinfo/info.xml.
The really crucial part in the webpack.config.js is

{ publicPath: path.join('/apps/', appName, '/js/'), }

My understanding is that this povides the path to fetch the js files from. I'm pretty sure which path is understood by the server does not depend on the package.json but on the appinfo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't that public path anyways overwritten by __webpack_public_path__ as the /apps/ will not work as a static location in case multiple app directories are used?

Copy link
Contributor

@skjnldsv skjnldsv May 19, 2022

Choose a reason for hiding this comment

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

My understanding is that this povides the path to fetch the js files from. I'm pretty sure which path is understood by the server does not depend on the package.json but on the appinfo.

We expect developers to use the same name in npm pkg AND appinfo :)
Not all repo using webpack-vue-config are apps btw, so no appinfo here 😉

max-nextcloud added a commit to nextcloud/text that referenced this pull request May 23, 2022
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 <max@nextcloud.com>
max-nextcloud added a commit to nextcloud/text that referenced this pull request May 23, 2022
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 <max@nextcloud.com>
max-nextcloud added a commit to nextcloud/text that referenced this pull request May 23, 2022
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 <max@nextcloud.com>
max-nextcloud added a commit to nextcloud/text that referenced this pull request May 31, 2022
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 <max@nextcloud.com>
max-nextcloud added a commit to nextcloud/text that referenced this pull request May 31, 2022
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 <max@nextcloud.com>
max-nextcloud added a commit to nextcloud/text that referenced this pull request May 31, 2022
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 <max@nextcloud.com>
max-nextcloud added a commit to nextcloud/text that referenced this pull request May 31, 2022
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 <max@nextcloud.com>
max-nextcloud added a commit to nextcloud/text that referenced this pull request May 31, 2022
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 <max@nextcloud.com>
max-nextcloud added a commit to nextcloud/text that referenced this pull request Jun 1, 2022
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 <max@nextcloud.com>
max-nextcloud added a commit to nextcloud/text that referenced this pull request Jun 7, 2022
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 <max@nextcloud.com>
max-nextcloud added a commit to nextcloud/text that referenced this pull request Jun 7, 2022
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 <max@nextcloud.com>
@max-nextcloud
Copy link
Contributor Author

As said in nextcloud/standards#3 (comment) we prefer to have two package.json now to also avoid confusion with dependencies

@skjnldsv skjnldsv deleted the feature/strip-nextcloud-scope-from-name branch August 29, 2022 17:06
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.

None yet

5 participants