Skip to content

Fix Tab default values#840

Merged
skjnldsv merged 1 commit into
masterfrom
fix/sidebar/Tab-default-values
Jan 28, 2020
Merged

Fix Tab default values#840
skjnldsv merged 1 commit into
masterfrom
fix/sidebar/Tab-default-values

Conversation

@skjnldsv

@skjnldsv skjnldsv commented Jan 27, 2020

Copy link
Copy Markdown
Contributor

After #833

vue.runtime.esm.js:1888 TypeError: Cannot read property 'name' of undefined
    at a.default (AppSidebarTab.js:formatted:1)
    at vue.runtime.esm.js:1666
    at Pt (vue.runtime.esm.js:1619)
    at i (vue.runtime.esm.js:4665)
    at vue.runtime.esm.js:4698
    at gn (vue.runtime.esm.js:4639)
    at a.t._init (vue.runtime.esm.js:5006)
    at new a (vue.runtime.esm.js:5154)
    at vue.runtime.esm.js:3283
    at init (vue.runtime.esm.js:3114)

@skjnldsv skjnldsv added bug Something isn't working 3. to review Waiting for reviews regression Regression of a previous working feature feature: app-sidebar Related to the app-sidebar component labels Jan 27, 2020
@skjnldsv skjnldsv added this to the 1.2.9 milestone Jan 27, 2020
@skjnldsv skjnldsv requested a review from juliusknorr January 27, 2020 15:18
@skjnldsv skjnldsv self-assigned this Jan 27, 2020
@juliusknorr

juliusknorr commented Jan 28, 2020

Copy link
Copy Markdown
Contributor

Hm, I tough I could work around changing the API, i thought this has worked when testing back then. 🙈

@georgehrke

Copy link
Copy Markdown
Contributor

This would introduce a breaking change that AppSidebarTab requires id from now on.
What about having id as optional property and using a computed property to returns id or if its null use this.name.toLowerCase().replace(/ /g, '-')?

@skjnldsv

skjnldsv commented Jan 28, 2020

Copy link
Copy Markdown
Contributor Author

This would introduce a breaking change that AppSidebarTab requires id from now on.
What about having id as optional property and using a computed property to returns id or if its null use this.name.toLowerCase().replace(/ /g, '-')?

I would say no because using the name (which is translated in every nc app) is definitely not a good thing and will for sure break.
Let's force id and make sure devs knows they have to provide it (it's just a string, let's not over react, 😁)

@ChristophWurst

Copy link
Copy Markdown
Contributor

I agree that this is a breaking change. You can't use this new code without modifying your app's code to add an id.

Let's force id and make sure devs knows they have to provide it

Yes, with a major version bump. Otherwise they might assume their existing code is still compatible.

@skjnldsv skjnldsv merged commit 91033f9 into master Jan 28, 2020
@skjnldsv skjnldsv deleted the fix/sidebar/Tab-default-values branch January 28, 2020 15:55
@skjnldsv

Copy link
Copy Markdown
Contributor Author

/backport to stable0-12

@backportbot-nextcloud

Copy link
Copy Markdown

backport to stable0-12 in #843

@ChristophWurst

Copy link
Copy Markdown
Contributor

Why was this breaking change released as a minor update? https://github.com/nextcloud/nextcloud-vue/releases/tag/v1.3.0

@ChristophWurst

Copy link
Copy Markdown
Contributor

As the majority of packages follow semver, I don't test minor bumps. Therefore I've set Dependabot to merge minor releases automatically. If I don't have tests that cover the usage of this changed component it will happily merge the PR and I have a broken app.

Please let's stick to semantical versions. I don't want to have to check the full change log for every version bump. If we have a breaking change so be it. Then app devs have to fix them. There is no real need to stick to the same major version for ages.

@skjnldsv

Copy link
Copy Markdown
Contributor Author

Yeah, we never did it that well on the components. I'll try to be more careful

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews bug Something isn't working feature: app-sidebar Related to the app-sidebar component regression Regression of a previous working feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants