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.

Sign up for GitHub

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

Jump to bottom

[FIX] Detect library namespace automatically #255

Merged
merged 6 commits into from
Jun 13, 2019
Merged

[FIX] Detect library namespace automatically #255

merged 6 commits into from
Jun 13, 2019

Conversation

RandomByte
Copy link
Member

@RandomByte RandomByte commented May 22, 2019 •
edited
Loading

Like already done for applications, read namespace information from "sap.app".id
attribute of manifest.json.

Before, some tasks derived the namespace from a libraries name.
This contract was not documented and not enforced. Causing some confusion when
using those tasks.

If no manifest.json is present, fall back to library.name attribute of .library.
If .library is not present, fall back to the relative path of the library.js file.
If that is not present, the namespace is undefined and some tasks are never
executed for that library:

  • generateJsdoc
  • executeJsdocSdkTransformation
  • generateManifestBundle

namespaces might become mandatory in a future major release.

@RandomByte RandomByte changed the title WIP [INTERNAL] Detect library namespace from manifest.json May 22, 2019
@RandomByte RandomByte force-pushed the detect-library-namespace branch 4 times, most recently from a4bcc00 to 3577bdd Compare May 22, 2019 21:55
@RandomByte RandomByte marked this pull request as ready for review May 22, 2019 21:56
@RandomByte
Copy link
Member Author

CC: just fyi @Thodd, @petermuessig

@coveralls
Copy link

coveralls commented May 22, 2019 •
edited
Loading

Coverage Status

Coverage increased (+0.4%) to 85.858% when pulling 7170dd2 on detect-library-namespace into 389e6c8 on master.

codeworrior
codeworrior reviewed May 23, 2019
View reviewed changes
lib/types/AbstractUi5Formatter.js
*
* @abstract
*/
class AbstractUi5Formatter extends AbstractFormatter {
Copy link
Member

Choose a reason for hiding this comment

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

Seeing 'Ui5' hurts my eyes...

Copy link
Member

Choose a reason for hiding this comment

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

Mine too 🙄

Copy link
Member

Choose a reason for hiding this comment

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

Hmm so what about this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Later in some other change 😬

It's private API anyways

matz3
matz3 requested changes May 23, 2019
View reviewed changes
lib/types/AbstractUi5Formatter.js Outdated Show resolved Hide resolved
lib/types/library/LibraryFormatter.js Show resolved Hide resolved
lib/types/library/LibraryFormatter.js Show resolved Hide resolved
lib/types/AbstractUi5Formatter.js
*
* @abstract
*/
class AbstractUi5Formatter extends AbstractFormatter {
Copy link
Member

Choose a reason for hiding this comment

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

Mine too 🙄

RandomByte added a commit to SAP/ui5-fs that referenced this pull request May 27, 2019
@RandomByte
resourceFactory: Add callback for virtual base path prefix determination
56b85e9 
The useNamespace flag was not sufficient in cases where the namespace
should only be added for projects that do not already represent it in
their directory structure.

With SAP/ui5-builder#255 libraries will have a
namespace property even though their directory structure already
represents the namespace.
RandomByte added a commit to SAP/ui5-fs that referenced this pull request May 29, 2019
@RandomByte
[FEATURE] AbstractAdapter: Add excludes option (#140)
daef31f 
- Update globby and micromatch dependencies
- Adapt to globby/micromatch changes
    - globby: 'nodir' option renamed to 'onlyFiles'
    - globby: Now transforms empty GLOB strings into globstar (matching
      everything instead of nothing)
        - mitigated by not globbing with empty strings anymore
    - globby/micromatch: Including excluded paths is now possible
        - before this was inconsistent
    - FileSystem._runGlob now throws errors for glob errors (instead of
      catching and logging them)
- resourceFactory: Add callback for virtual base path prefix determination
    - The useNamespace flag was not sufficient in cases where the namespace
      should only be added for projects that do not already represent it in
      their directory structure.
      With SAP/ui5-builder#255 libraries will have a
      namespace property even though their directory structure already
      represents the namespace.
@RandomByte
[INTERNAL] Detect library namespace from manifest.json
5a15391 
If no manifest.json is present, fallback to .library.

Also refactor read operations of UI5 specific, formatter relvevant
files.
@RandomByte
Split getManifest implementation to Application- and LibraryFormatters
2be9fd0
@RandomByte RandomByte force-pushed the detect-library-namespace branch 2 times, most recently from e6b5a5f to 5a26c6f Compare June 13, 2019 13:50
@RandomByte
Copy link
Member Author

Reminder to myself: Squash it

@RandomByte RandomByte changed the title [INTERNAL] Detect library namespace from manifest.json [FIX] Detect library namespace automatically Jun 13, 2019
@RandomByte RandomByte merged commit 604d4d3 into master Jun 13, 2019
@RandomByte RandomByte deleted the detect-library-namespace branch June 13, 2019 18:39
@RandomByte RandomByte restored the detect-library-namespace branch June 13, 2019 20:42
@RandomByte RandomByte deleted the detect-library-namespace branch June 14, 2019 07:55
@nlunets
Copy link
Member

nlunets commented Jun 25, 2019

@codeworrior @RandomByte @matz3 this change broke building application with all dependencies.

The main reason being that you now expect a namespace to be provided for libraries, and you want to read that from the LibraryFormatter...
However the library formatter is never called (neither is the application formatter for that matter), and apparently it has never been called :)

So right now you cannot even build the testsuite with that, this is what's breaking ui5-typescript master branch as well @bd82 FYI

@RandomByte
Copy link
Member Author

@nlunets thanks for reporting. Could you please share some additional information?

  • What is broken? Is there an error message? Are artefacts missing from the build result?
  • What version of the UI5 CLI are you referring to?
  • How can we reproduce the issue?

I just did successful builds (ui5 build --all) of the latest master of the OpenUI5 Sample App and the OpenUI5 Testsuite using the UI5 CLI v1.5.3.

Note that there have been follow up changes to this one resolving some issues we identified after merging this PR:

However, they all got released with the same UI5 Builder v1.3.1 release:
v1.3.0...v1.3.1

@nlunets
Copy link
Member

nlunets commented Jun 25, 2019 

info builder:builder library sap.ui.core Skipping some tasks due to missing library namespace information. Your project might be missing a manifest.json or .library file. Also see: https://github.com/SAP/ui5-builder#library
info builder:builder library sap.ui.core 🔨 (1/1) Running task buildThemes...

There should be some extra steps here

info builder:builder library sap.ui.core 🔨 (1/3) Running task generateJsdoc...
WARNING: The @private tag does not permit a value; the value will be ignored. File: ManagedObjectModel.js, line: 497
WARNING: The @private tag does not permit a value; the value will be ignored. File: ManagedObjectModel.js, line: 506
info builder:builder library sap.ui.core 🔨 (2/3) Running task executeJsdocSdkTransformation...

@nlunets
Copy link
Member

nlunets commented Jun 25, 2019 •
edited
Loading

Before the namespace was computed from the name

const namespace = project.metadata.name.replace(/\./g, "/");

https://github.com/SAP/ui5-builder/pull/255/files#diff-28ca54829f3739e6f1ab3fae3ae18940L22

And now it's just not coming from anywhere because we're never going into the library / application formatter.

@RandomByte
Copy link
Member Author

The yarn.lock file in the OpenUI5 repository references old versions of some UI5 Tooling modules that seemingly lead to this issue. We need to investigate why this happens exactly.

In the meantime, please install the UI5 CLI globally and remove the local installation. This should solve the issue.

@nlunets
Copy link
Member

nlunets commented Jun 25, 2019

yep it works with removing the yarn.lock file apparently, thanks :)
Still, i'm not sure the formatter is called ;)

@RandomByte
Copy link
Member Author

RandomByte commented Jun 25, 2019 •
edited
Loading

It is, but in the erroneous setup an old version of the UI5 Builder is used that does not have the new namespace detection that got implemented in this PR.

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

@codeworrior codeworrior codeworrior left review comments

@matz3 matz3 matz3 approved these changes

@devtomtom devtomtom Awaiting requested review from devtomtom

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@RandomByte @coveralls @nlunets @matz3 @codeworrior