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

[Vue] enable full typescript compilation + fix typing errors in existing Vue code #18545

Merged
merged 603 commits into from Jan 10, 2022

Conversation

diosmosis
Copy link
Member

@diosmosis diosmosis commented Dec 27, 2021

Description:

This PR is based off of #18446.

By default Vue sets the 'transpile only' option in the TypeScript compiler which only does a cursory check of typing related errors. This means the current code has a lot of typing errors (and also some bugs) that were never detected. It also means we can't output typing information for plugin UMDs, so type information for CoreHome would be lost when using CoreHome exports in another plugin. This PR fixes that. Note: Vue doesn't actually support doing a full compile natively (there's at least one open issue), so we workaround it.

(This is also why typescript errors in unit tests do not appear in vue:build.)

Changes:

  • In vue.config.js if in production mode, turn on full compilation. In development mode this is not done so development will still be fast (using full type checking takes a bit longer since tsc will do more work).
  • Output typings for plugin UMDs to the /path/to/matomo/@types directory. These will be picked up by a change to tsconfig.json and typescript will be able to apply typing information to JS imported from other plugins.
  • Fixes for all typing errors. Some are superficial, but there are some actual bug fixes in here as well.
    Note: one "fix" is a workaround for vue's DeepReadonly type not being able to handle recursive interfaces (eg, our Widget data structure). Fixed by separating the part that recurses and casting to a sub-interface when needed.

Review

diosmosis and others added 17 commits December 30, 2021 15:34
* deprecate support in vue for FormField.allSettings since deep watching the property doesnt quite work

* built vue files
* Added "What is new" notification display, populated by a new event

* Removed test example event hook

* Added support for applying a link attribute to menu items, fixes layout issue for mobile with html menu items

* Updated UI test screenshots

* Revert accidental edit

* Hide the "What's new" icon if there are no new features to show

* Changed to use changes.json, track user last viewed, added ui test

* Fix UserManager unit tests broken by new ts_changes_viewed user field

* Moved getChanges to separate helper class, added unit test, added user view access check

* Updated to add new changes table and populate only on plugin update/install

* Added missing fixture class, updated UI screenshots

* Updated matomo font to add ringing bell and new releases icons

* Fix for integration test

* Reworked class structure, removed unnecessary angular directive, merged templates, other tidy ups

* built vue files

* built vue files

* Added null user check, missing table exception handling, show plugin name in change title, better handling of missing change fields

* Added sample changes file, moved UserChanges db code to changes model, added return type hints, better db error code handling, various other improvements

* Revert accidental UI screenshot commit

* Fix for incorrect link name parameter in sample changes, switched back to using $db->query for INSERT IGNORE

* Integration test fix, UI screenshot updates

* Test fix

* Added link styling, show CoreHome changes without plugin prefix in title

* Update UI test screenshot

* Added styles to the popover, added event for filtering changes

* Test fix

* UI test screenshot updates

Co-authored-by: sgiehl <stefan@matomo.org>
Co-authored-by: bx80 <bx80@users.noreply.github.com>
update a test failed XML
Co-authored-by: diosmosis <diosmosis@users.noreply.github.com>
* Translated using Weblate (Greek)

Currently translated at 100.0% (162 of 162 strings)

Translation: Matomo/Plugin CoreAdminHome
Translate-URL: https://hosted.weblate.org/projects/matomo/plugin-coreadminhome/el/

[ci skip]

Co-authored-by: Hosted Weblate <hosted@weblate.org>
Co-authored-by: Vasilis Lourdas <dev@lourdas.eu>

* Translated using Weblate (Chinese (Simplified))

Currently translated at 83.9% (136 of 162 strings)

Translation: Matomo/Plugin CoreAdminHome
Translate-URL: https://hosted.weblate.org/projects/matomo/plugin-coreadminhome/zh_Hans/

[ci skip]

Translated using Weblate (Chinese (Simplified))

Currently translated at 99.6% (620 of 622 strings)

Translation: Matomo/Matomo Base
Translate-URL: https://hosted.weblate.org/projects/matomo/matomo-base/zh_Hans/

[ci skip]

Co-authored-by: Hosted Weblate <hosted@weblate.org>
Co-authored-by: 刘韬 <lyuutau@outlook.com>

* Update translation files

Updated by "Squash Git commits" hook in Weblate.

Translation: Matomo/Plugin CoreAdminHome
Translate-URL: https://hosted.weblate.org/projects/matomo/plugin-coreadminhome/

[ci skip]

Co-authored-by: Vasilis Lourdas <dev@lourdas.eu>
Co-authored-by: 刘韬 <lyuutau@outlook.com>
* update files

* sidenav start

* make getRef a utility method

* tweak

* add return type

* finish converting side-nav directive

* starting on reporting menu conversion

* remove unused properties

* convert reporting pages service

* migrate report metadata store

* remove angularjs files

* migrating reporting pages store

* make store adapters more immutable

* get service adapters to work

* fix a UI test

* another html fix

* migrate most of reporting menu directive and model

* Use themed font family for input forms to override materialize.css styling

* rebuild vue

* add a missing div

* ui test fixes

* update styling

* get to build

* get to load in the UI w/o error

* clone result of functions

* fix compile issue

* migrate widget loader and get to load in UI

* rebuild vue

* migrate widgetcontainer

* migrate widget bydimension container

* migrate widget + add tooltips directive

* quick fix

* Updating version to 4.6.0

* loading in page

* update expected screenshot

* add wait just in case travis is slow

* fix ordering bug

* add another wait

* rebuild vue

* css tweak

* fix some bugs and tests

* undo screenshot changes

* Menus test passing locally

* [Vue] date picker viewDate property is not kept up to date (#18385)

* viewDate ref is not kept up to date

* rebuild corehome

* reporting menu subcategory items are meant to be normal links

* update some screenshots

* use innerText instead of text() since angularjs maintains newlines in HTML that vue does not add

* trigger angularjs digest after ajaxhelper request

* rebuild vue

* update screenshots, fix bug in link generation in reporting menu and allow syncing multiple screenshot regexes at a time

* undo box-shadow change for UI tests

* fix more issues & update more tests

* update some screenshots

* fix some tests

* rebuild CoreHome

* quick fix

* built vue files

* fix angularjs issue

* add comment

* update umd files

* 4.6.1-rc1

* 4.6.1

* fix field array title

* apply some pr feedback

* apply more pr feedback

* another fix

* tweak

* fix ng-change not executed before ng-model

* fix another set of issues

* fix another issue

* rebuild vue

* better ng-change/ng-model fix

* update some screenshots

* rebuild vue

* remove some TODOs

* initiate initial ng-change ONLY for site selectors where this behavior applies

* emit/broadcast on correct scope in wrapper

* rebuild vue

* fix some issues

* couple more fixes

* fix another title issue

* rebuild vue

* do not report on ajax errors in notifications if not logged in

* migrate reporting page and model

* rebuild vue

* create sites selector model adapter

* fix siteselector vue bug, initial site is only set if there is just one site available

* rebuild vue

* migrate plugin settings directive

* remove TODO

* migrate plugin filter directive

* migrate two more plugins directives

* migrate save button

* fix a bunch of bugs

* fix another widget bug

* allow change event name between angularjs and vue

* rebuild vue

* migrate plugin form directive

* get to work

* migrate select-on-focus directive and start migrating report-export directive

* finish migrating report export directive & popover component + create reusable function to create vue app and add globals to it

* rebuild vue

* remove angularjs files and move less contents to vue dir

* built vue files

* fix function signature

* fix vue warning

* fix ajax request race condition

* rebuild vue

* add new notification type "help" so the help notification is not cleared when clearing transient notifications

* fix some bugs and tests

* update screenshot

* update screenshot & fix a test

* allow using unminified jquery ui + fix bug in last fix

* fix error when enrichedheadline is used in modal

* add polyfill min.js

* remove two todos

* fix widget url logic

* update some screenshots and fix sanitization/escape issue

* update screenshots

* rebuild vue

* fix url location updating regression in MatomoUrl.updateLocation use

* submodule

* update screenshots and fix possible error in json parse

* built vue files

* Merge branch 'vue-period-selector-regression' into vue-reporting-menu

* rebuild vue

* use correct variable

* rebuild vue

* fix widget url logic

* segment parameter can be undefined now for some reason

* fix ngmodel binding in siteselector adapter (for last time hopefully)

* the original site selector only set the first site to the first site in the initial sites query if there was only one site in the entire matomo instance

* fix sitesmanager ui test failure

* fix usersettings test failure

* rebuild vue

* more siteselector tweaks.

* build CoreHome

* more siteselector tweaks.

* another siteselector issue

* update screenshots

* update screenshot and try to fix random failure

* fix some issues in widget.vue when containerid is specified

* fix couple tests

* fix several test failures

* fix string concat

* fix test failure

* extra change

* fix last change and random failure

* styling fix

* fix last fix

* real fix this time

* fix stray request

* proper fix

* update build files

* try to fix random failure

* do not submit form

* check for api errors in promise chain in ajaxhelper.ts

* force a digest after a location change

* use proper abortcontroller method instead of promise hack, have to add new polyfill + try to fix random test failure

* some UI test fixes

* fix some report export issues

* several save button fixes + make replace approximation in createAngularJsAdapter better

* apply after manual click triggering in savebutton

* add names to divs so they can still be queried as they were in angularjs

* rebuild vue

* now that format_metrics checkbox works, need to check it

* fix unintended changes

* updated expected screenshots

* update two more

* go back to previous format_metrics behavior in popover

Co-authored-by: Justin Velluppillai <justin@innocraft.com>
Co-authored-by: justinvelluppillai <justinvelluppillai@users.noreply.github.com>
Co-authored-by: Matthieu Aubry <mattab@users.noreply.github.com>
* deprecate support in vue for FormField.allSettings since deep watching the property doesnt quite work

* built vue files
@diosmosis diosmosis added the Needs Review PRs that need a code review label Dec 31, 2021
@diosmosis diosmosis marked this pull request as ready for review December 31, 2021 03:17
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

I had a look through all the changes. As I'm not very experienced with typescript I can't really assess if all those changes would be needed or what they are good for. But as no tests are failing I guess it should be good.
I also clicked roughly through the UI, but for sure didn't retest every feature that had been migrated before. So in theory there could be some regressions introduced I didn't see.

@@ -78,7 +79,7 @@ export default defineComponent({
}
}, 0);

if (this.actualFeature && (this.actualFeature === true || this.actualFeature === 'true')) {
if (this.actualFeature && this.actualFeature === 'true') {
Copy link
Member

Choose a reason for hiding this comment

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

Was that change on purpose? Wondering as it doesn't seem the same logically.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, actualFeature is initialized to this.feature which is typed to be a String, so the value will never be a boolean true. I don't think it was possible in angularjs either since the binding value was @, not = or <. Typescript complained about this being unnecessary if I recall correctly.

@diosmosis
Copy link
Member Author

@sgiehl

As I'm not very experienced with typescript I can't really assess if all those changes would be needed or what they are good for.

You can see why a change was made doing the following:

  • revert the change locally
  • run vue:build $pluginName. tsc will fail with an error and that will tell you why the change was made.

This PR is just fixing typing errors that tsc complains about when you set it to actually use and generate typing information (by default vue does not enable this, so much of the typescript code that was written was incorrect). This PR would not be large or even necessary if this setting change was done in the beginning, but vue does not seem to mention that it doesn't do real type checking.

@sgiehl
Copy link
Member

sgiehl commented Jan 10, 2022

Yeah sure. Feel free to merge it then. If any regressions may pop up later we can fix them then as well. I don't think it's needed to test everything in detail again now

@diosmosis diosmosis merged commit 40afe8b into 4.x-dev Jan 10, 2022
@diosmosis diosmosis deleted the vue-no-passthrough-transpile branch January 10, 2022 22:52
@justinvelluppillai justinvelluppillai added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants