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

Several instances of undeclared dependency usage #2584

Closed
jbragiel opened this issue Jul 12, 2019 · 5 comments
Closed

Several instances of undeclared dependency usage #2584

jbragiel opened this issue Jul 12, 2019 · 5 comments
Assignees
Labels

Comments

@jbragiel
Copy link

OpenUI5 version: 1.60 (but checked that issue is still present in master branch)

Browser/version (+device/version): N/A

Any other tested browsers/devices(OK/FAIL): N/A

URL (minimal example if possible): N/A

User/password (if required and possible - do not post any confidential information here): N/A

Steps to reproduce the problem:

  1. Open one of the referenced files from the list below. The file is the line with the complete path and js extension.
  2. Search for one of the modules under the file name in the list below.
  3. See that the API dependency isn't being required/imported properly.

What is the expected result?
Dependency modules are imported before they are used.

What happens instead?
In the cases listed below, APIs from the modules are used without importing them first.

Any other information? (attach screenshot if possible)
I tried to leave out any non-runtime file from the list (like support, demokit, documentation,
unit test, etc.). There are a few files in the list I wasn't sure about so I left them in.

=== sap.m ===
src/sap.m/src/sap/m/FacetFilter.js
sap.m.Popover
sap.m.Button
sap.m.SearchField
sap.ui.model.FilterOperator
sap.m.Page
sap.m.Bar
sap.m.Dialog
sap.m.List
sap.m.StandardListItem
sap.ui.core.CustomData
sap.ui.model.json.JSONModel
sap.m.CheckBox
sap.m.Text
sap.m.Toolbar
sap.m.ToolbarSpacer

src/sap.m/src/sap/m/FacetFilterList.js
sap.ui.model.FilterOperator
sap.ui.model.FilterType

src/sap.m/src/sap/m/ios7.js
sap.ui.base.EventProvider
sap.ui.Device
sap.ui.base.EventProvider
sap.m._Ios7._rTagRegex.test
sap.m._Ios7

src/sap.m/src/sap/m/LabelRenderer.js
sap.ui.core.LabelEnablement
sap.ui.core.VerticalAlign

src/sap.m/src/sap/m/Menu.js
sap.ui.core.Popup.Dock

src/sap.m/src/sap/m/OverflowToolbarRenderer.js
sap.m.BarInPageEnabler

src/sap.m/src/sap/m/PanelRenderer.js
sap.ui.Device

src/sap.m/src/sap/m/PDFViewerRenderer.js
jquery.sap.global (It provides jQuery.sap.log.warning)

src/sap.m/src/sap/m/SegmentedButton.js
sap.m.Button
sap.m.Image

src/sap.m/src/sap/m/ViewSettingsDialog.js
sap.m.Dialog
sap.m.Button
sap.m.ButtonType
sap.m.NavContainer
sap.m.Label
sap.m.Bar
sap.m.SegmentedButton
sap.m.Page
sap.m.ViewSettingsItem

src/sap.m/src/sap/m/ViewSettingsPopover.js
sap.m.CheckBox
sap.m.SegmentedButtonItem
sap.m.Label
sap.m.SearchField

=== sap.tnt ===
src/sap.tnt/src/sap/tnt/NavigationListItem.js
sap.ui.core.TextDirection
sap.ui.core.TextAlign

src/sap.tnt/src/sap/tnt/SideNavigation.js
sap.ui.base.ManagedObject

src/sap.tnt/src/sap/tnt/ToolHeader.js
sap.ui.Device
sap.m.PlacementType

src/sap.tnt/src/sap/tnt/ToolPageRenderer.js
sap.ui.Device

=== sap.ui.commons ===
src/sap.ui.commons/src/sap/ui/commons/DatePicker.js
sap.ui.unified.Calendar
sap.ui.unified.DateRange

=== sap.ui.core ===
src/sap.ui.core/src/sap/ui/core/BusyIndicatorUtils.js
sap.ui.core.BusyIndicatorSize

src/sap.ui.core/src/sap/ui/core/message/Message.js
sap.ui.core.MessageType

src/sap.ui.core/src/sap/ui/core/message/MessageManager.js
sap.ui.core.MessageType

src/sap.ui.core/src/sap/ui/core/message/MessageMixin.js
sap.ui.core.LabelEnablement

=== sap.ui.dt ===
src/sap.ui.dt/src/sap/ui/dt/ContextMenuControl.js
sap.ui.Device

src/sap.ui.dt/src/sap/ui/dt/plugin/CutPaste.js
sap.ui.Device

src/sap.ui.dt/src/sap/ui/dt/plugin/DragDrop.js
sap.ui.Device

=== sap.ui.layout ===
src/sap.ui.layout/src/sap/ui/layout/AssociativeSplitter.js
sap.ui.layout.SplitterLayoutData

src/sap.ui.layout/src/sap/ui/layout/Splitter.js
sap.ui.layout.SplitterLayoutData

=== sap.ui.unified ===
src/sap.ui.unified/src/sap/ui/unified/calendar/DatesRowRenderer.js
sap.ui.core.CalendarType

src/sap.ui.unified/src/sap/ui/unified/calendar/MonthsRow.js
sap.ui.unified.DateRange

src/sap.ui.unified/src/sap/ui/unified/calendar/TimesRow.js
sap.ui.unified.DateRange

src/sap.ui.unified/src/sap/ui/unified/CalendarDateInterval.js
sap.ui.unified.DateRange

src/sap.ui.unified/src/sap/ui/unified/CalendarLegend.js
sap.ui.unified.CalendarLegendItem

src/sap.ui.unified/src/sap/ui/unified/CalendarMonthInterval.js
sap.ui.unified.DateRange

src/sap.ui.unified/src/sap/ui/unified/CalendarOneMonthInterval.js
sap.ui.unified.DateRange

src/sap.ui.unified/src/sap/ui/unified/CalendarRow.js
sap.ui.unified.CalendarAppointment

src/sap.ui.unified/src/sap/ui/unified/CalendarTimeInterval.js
sap.ui.unified.DateRange
sap.ui.unified.Calendar

src/sap.ui.unified/src/sap/ui/unified/CalendarWeekInterval.js
sap.ui.unified.DateRange

=== sap.ui.ux3 ===
src/sap.ui.ux3/src/sap/ui/ux3/DataSetSimpleView.js
sap.ui.model.ChangeReason

src/sap.ui.ux3/src/sap/ui/ux3/Notifier.js
sap.ui.base.EventProvider

@codeworrior
Copy link
Member

Some of those are intentional, most of them are not. I've run the ui5-migration tooling on sap.ui.core, sap.tnt, sap.ui.unified, sap.ui.dt, sap.ui.layout, sap.ui.ux3, sap.ui.commons and manually reduced the modifications to those that seem trivial (change is pending).

Several of the findings in sap.m need more work or judgement by the corresponding control developers and I'll leave them open for now, but update the issue soon.

@codeworrior
Copy link
Member

codeworrior commented Jul 15, 2019

Change has been submitted and fixed most of the issues above. I reopen the ticket as there are some left overs:

=== sap.m ===
src/sap.m/src/sap/m/FacetFilter.js
--> control should be refactored and lazy load the additional controls when needed

src/sap.m/src/sap/m/SegmentedButton.js
--> usages of IconPool.createControlByURI in general suffer from the problem, that configuration at runtime decides whether the given class is needed or not

src/sap.m/src/sap/m/ViewSettingsDialog.js
--> control should be refactored and lazy load the additional controls when needed

src/sap.m/src/sap/m/ViewSettingsPopover.js
--> control should be refactored and lazy load the additional controls when needed

=== sap.ui.commons ===
src/sap.ui.commons/src/sap/ui/commons/DatePicker.js
--> commons lazily loads sap.ui.unified and only then requires Calendar and DateRange

src/sap.ui.core/src/sap/ui/core/message/Message.js
--> MessageType needs to be extracted to a module of its own before a hard dependency can be created in Message.js. Otherwise, there'll be a cyclic dependency which is hard to handle during bootstrap

src/sap.ui.core/src/sap/ui/core/message/MessageManager.js
--> MessageType needs to be extracted to a module of its own before a hard dependency can be created in MessageManager.js. Otherwise, there'll be a cyclic dependency which is hard to handle during bootstrap

src/sap.ui.core/src/sap/ui/core/message/MessageMixin.js
--> this I left out because I thought it has the same issues as the previous two, but that'S wrong. Should be fixed.

@codeworrior codeworrior reopened this Jul 15, 2019
@jbragiel
Copy link
Author

Thank you Frank. I merged the commit for this issue into our fork of the 1.60 SDK. The only change that seemed odd to me was the removal of the jquery.sap.global import from src/sap.m/src/sap/m/ShellRenderer.js. Wouldn't the use of the jQuery.sap.require API in that ShellRenderer trigger jQuery.sap.stubs to load jquery.sap.global?

@codeworrior
Copy link
Member

If we would be able to boot the framework already without the monolithic Core.js, then yes. But Core.js is still mandatory and has a dependency to jquery.sap.global.js (for compatibility reasons), so jQuery.sap.require should be available in all relevant scenarios.

On the other side, relying on transitive dependencies is considered bad style, so maybe we should re-add the dependency or add Parameters as a top-level dependency.

@flovogt
Copy link
Member

flovogt commented Jan 18, 2024

Old issue but the remaining findings will be gone with UI5 2.X. Therefore closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants