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

[FIX] manifestCreator: Only consider component files called Component.js #273

Merged
merged 3 commits into from
Sep 20, 2019

Conversation

RandomByte
Copy link
Member

In the past files called *Component.js where also considered as
Components during manifest creation.

@RandomByte RandomByte requested a review from codeworrior June 24, 2019 12:35
@RandomByte RandomByte force-pushed the manifest-creator-component-js branch from 5beb444 to 8d82f22 Compare June 24, 2019 12:50
@RandomByte RandomByte removed the request for review from codeworrior June 24, 2019 12:53
In the past files called *Component.js where also considered as
Components during manifest creation.
@RandomByte RandomByte force-pushed the manifest-creator-component-js branch from 8d82f22 to ba9da2e Compare June 24, 2019 12:54
@coveralls
Copy link

coveralls commented Jun 24, 2019

Coverage Status

Coverage increased (+0.7%) to 86.618% when pulling 2e3149a on manifest-creator-component-js into 31e43b5 on master.

@@ -159,7 +159,7 @@ async function createManifest(libraryResource, libBundle, descriptorVersion, _in
function findComponentPaths() {
const result = [];
const prefix = libraryResource.getPath().slice(0, - ".library".length);
const components = libBundle.getResources(/(?:[^/]+\/)*Component\.js$/);
const components = libBundle.getResources(/^(?:[^/]*\/)*Component\.js$/);
Copy link
Member

Choose a reason for hiding this comment

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

The anchor was for sure missing, but the remainder of the pattern looks strange to me now. It accepts empty path segments (which shouldn't exist in the wild) and as the bundler is for libraries, I also don't know why a /Component.js should exist. It would have no namespace, which would be an error for a manifest.json.

Copy link
Member

Choose a reason for hiding this comment

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

@codeworrior this code actually detect all components and will make the manifest creator complain if there is both a library and a Component.js.

Right now this create the following errors:

ERR! builder:processors:manifestCreator Package /resources/sap/fe/AppComponent.js contains both '*.library' and 'Component.js'. This is not supported by manifests, therefore the component won't be listed in the library's manifest.

/^(?:[^/]*\/)*Component\.js$/ matches things like

  • Component.js
  • /Component.js
  • /toto/Component.js
  • /toto/roro/Component.js

Which actually is an issue because we for instance ship components internally that can be reused as part of the library...
What would be the correct behavior here ?

Copy link
Member

Choose a reason for hiding this comment

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

Actually after looking at this a bit more and discussing with @codeworrior we agreed that the right regexp would . be
^/(?:[^/]+\/)*Component\.js$ because it will find the right component files without matching an empty path in . the middle like the other one.
And since all resources path start with a slash that's coherent

@RandomByte RandomByte merged commit 82fe267 into master Sep 20, 2019
@RandomByte RandomByte deleted the manifest-creator-component-js branch September 20, 2019 10:45
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.

5 participants