[FEATURE] simplistic serveIndex with additional properties #23

petermuessig · 2018-07-06T16:01:05Z

@RandomByte @matz3 please take a look into the new serveIndex alternative for the ui5-server.

There are still some internals from the resource used when creating the resource info object. Maybe some of the internals can also become something for a more official API of the Resource but we could do that in a second step and provide the simplistic serveIndex to avoid the dependency to a github repo.

CLAassistant · 2018-07-06T16:01:12Z

All committers have signed the CLA.

coveralls · 2018-07-06T16:18:07Z

Pull Request Test Coverage Report for Build 102

35 of 44 (79.55%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+1.4%) to 73.75%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/middleware/serveIndex.js	33	42	78.57%

Totals
Change from base Build 97:	1.4%
Covered Lines:	255
Relevant Lines:	327

💛 - Coveralls

petermuessig · 2018-07-07T08:52:38Z

The tests may be a bit primitive but at least the logic is executed and a lightweight check for the result has been implemented.

codeworrior · 2018-07-07T13:17:30Z

package.json

@@ -102,7 +102,6 @@
 		"portscanner": "^2.1.1",
 		"prompt": "^1.0.0",
 		"replacestream": "^4.0.3",
-		"serve-index": "RandomByte/serve-index#fs-option",


I would have expected that when the package.json has changed, there also should be a new package-lock.json?

Ok, I did not check that the package-lock.json is submitted. I do not do this normally...

codeworrior · 2018-07-07T13:23:22Z

lib/middleware/serveIndex.js

+		size: formatSize(stat.size),
+		sizeInBytes: stat.size,
+		project: resource._project.id,
+		realPath: resource._project.path + resource.getPath()


Good point!

RandomByte · 2018-07-08T21:38:35Z

lib/middleware/serveIndex.js

+		} else if (!a.isDir && b.isDir) {
+			return 1;
+		} else {
+			return a.name.localeCompare(b.name);


localCompare doesn't do well when comparing numbers:

(Finder sorts correctly)

This is like a filesystem sorts the numbers! ;-)

But not how most file system browsers (Finder, Explorer, etc.) sort numbers. I find this sorting rather confusing. Maybe we could also check how Apache and others do this.

Mhmm, what sorting do you expect?

The sorting in Windows Explorer is not more than a "nice try" from my POV - I really hate it. Did you ever try to find a GUID in a folder full of GUIDS (some starting with digits, others not? It's a mess. You easily miss a file just because of the sorting. Simple alphanumeric sorting would be much better.

"Smart" sorting only makes sense when a folder only contains a single kind of names (only alpha or only numeric). But that's not how it is implemented, at least not in Windows Explorer.

+1 for strict ASCII sorting...

RandomByte · 2018-07-08T21:59:09Z

lib/middleware/serveIndex.js

+		size: formatSize(stat.size),
+		sizeInBytes: stat.size,
+		project: resource._project.id,
+		realPath: resource._project.path + resource.getPath()


This can't work. Appending the full virtual path of the resource to the physical root path of its project (not even the resources reader) can only produce valid paths by pure chance.

For /resources/sap-ui-core.js you currently get: [...]/node_modules/@openui5/sap.ui.core/resources/sap-ui-core.js instead of [...]/node_modules/@openui5/sap.ui.core/src/sap-ui-core.js.

There is currently no way to get the "physical" path of a resource. This is intentional as ui5-fs is explicitly not relying on physical file systems.

In general, I don't know whether we should expose full file paths here just for convenience. People could still use this for module on production systems and the file paths might represent security relevant information. How is this handled by Apache et.al.?

I spent a TODO comment to consider this but keep the logic for now

I will just use the project path

RandomByte · 2018-07-08T22:04:26Z

lib/middleware/serveIndex.js

+		lastModified: new Date(stat.mtime).toLocaleString(),
+		size: formatSize(stat.size),
+		sizeInBytes: stat.size,
+		project: resource._project.id,


The _project attribute is undocumented and commented as an Experimental, internal parameter. We should at least deal with it eventually being undefined (which it is for all dynamically generated resources).

As written in my comments before, we need to consider whether to expose some more additional information

RandomByte · 2018-07-08T22:07:33Z

lib/middleware/serveIndex.js

+ * @returns {String} HTML content for the resource listing
+ */
+function createContent(path, resourceInfos) {
+	return `<!DOCTYPE html>


I would love to have this as a Handlebars template. All with syntax highlighting and stuff. I'm ok with merging this as a start but would offer myself to move it into a proper template later on.

I would rather tend to do this as an enhancement in future

RandomByte · 2018-07-08T22:11:56Z

lib/middleware/serveIndex.js

+ * @param {Resource} resource the resource
+ * @returns {String} mime type
+ */
+function getMimeType(resource) {


Unit test for this function would be great

Did you do this for the serveResource? We should better externalize this in a util function... ;-)

RandomByte · 2018-07-08T22:12:02Z

lib/middleware/serveIndex.js

+ * @param {Number} bytes bytes
+ * @returns {String} human readable size
+ */
+function formatSize(bytes) {


Unit test for this function would be great

Util function would be better - but those kind of enhancements might be something for a general cleanup in a separate commit

RandomByte · 2018-07-08T22:12:08Z

lib/middleware/serveIndex.js

+ * @param {Resource} resource the resource to convert
+ * @returns {Object} resource info object
+ */
+function createResourceInfo(resource) {


Unit test for this function would be great

Internally called by serveIndex which runs through that function!

RandomByte · 2018-07-08T22:12:14Z

lib/middleware/serveIndex.js

+ * @param {Resource[]} resources an array of resources
+ * @returns {Object[]} sorted array of resource infos
+ */
+function createResourceInfos(resources) {


Unit test for this function would be great

Internally called by serveIndex which runs through that function! See line coverage...

RandomByte · 2018-07-08T22:12:23Z

lib/middleware/serveIndex.js

+ * @param {Object[]} resourceInfos an array of resource infos
+ * @returns {String} HTML content for the resource listing
+ */
+function createContent(path, resourceInfos) {


Unit test for this function would be great 😉

Internally called by serveIndex which runs through that function! See line coverage...

RandomByte · 2018-07-09T08:21:59Z

A rebase is necessary due to conflicts in package-lock.json

petermuessig · 2018-07-09T08:29:18Z

Grml - again a merge conflicT!

Fixes: https://github.com/SAP/ui5-server/issues/16

petermuessig · 2018-07-09T09:02:10Z

Rebase has been done

matz3 · 2018-07-09T09:17:14Z

I've added an export in index.js, so that it can be used directly.

petermuessig · 2018-07-09T10:51:39Z

@matz3 thanks for adding the serveIndex to the export - I did not see that place...

@RandomByte what about a merge now?

RandomByte

Fair enough I guess

RandomByte · 2018-07-09T11:14:31Z

Things I would like to follow up on:

Unit tests for most functions (should be required for all new features of the tooling)
Exposing local paths via server - security relevant?
Check internal Resource attributes _name and _project
Use HTML template
Make it look attractive (like it's from 2018)

RandomByte · 2018-07-09T12:44:29Z

lib/middleware/serveIndex.js

+				<td${info.isDir ? ">" : " title=\"" + info.sizeInBytes + " Bytes\">" + info.size}</td>
+				<td><a href="${info.path}">${info.name}</a></td>
+				<td>${info.mimetype}</td>
+				<td><a href="file:////${info.projectPath}" title="${info.projectPath}">${info.project}</a></td>


file:// URLs neither work in Chrome, nor Safari (didn't test others) for security reasons: https://support.google.com/gsa/answer/2664790?hl=en

Console error logged in both browsers after clicking such a link: Not allowed to load local resource. From what I found, only resources loaded via file:// can open such links.

I would opt for removing those links.

I would rather keep it as the link would be a server absolute link on mac which would point to the wrong location. At least copy and paste can work partially with the file URL...

Imho this is confusing. A non-clickable link with no explanation whatsoever.

Also, what is the benefit of opening a projects directory in your Browser?
I'm not aware how to open file:// links directly in the Finder or Terminal.

petermuessig requested review from RandomByte and matz3 July 6, 2018 16:01

petermuessig force-pushed the feature-serveIndex branch 2 times, most recently from 2860b59 to f4bff11 Compare July 6, 2018 16:13

petermuessig force-pushed the feature-serveIndex branch 2 times, most recently from 90bb04f to 996baab Compare July 6, 2018 20:03

petermuessig force-pushed the feature-serveIndex branch from 996baab to 9af6458 Compare July 7, 2018 09:08

petermuessig added the enhancement New feature or request label Jul 7, 2018

codeworrior reviewed Jul 7, 2018

View reviewed changes

RandomByte requested changes Jul 8, 2018

View reviewed changes

petermuessig force-pushed the feature-serveIndex branch 2 times, most recently from dcec1be to dcf5194 Compare July 9, 2018 08:06

petermuessig force-pushed the feature-serveIndex branch from dcf5194 to 7120b4f Compare July 9, 2018 08:28

[FEATURE] simplistic serveIndex with additional properties

2d6876b

Fixes: https://github.com/SAP/ui5-server/issues/16

petermuessig force-pushed the feature-serveIndex branch from 7120b4f to 2d6876b Compare July 9, 2018 08:33

Export serveIndex middleware

664b98e

RandomByte approved these changes Jul 9, 2018

View reviewed changes

RandomByte reviewed Jul 9, 2018

View reviewed changes

petermuessig merged commit b96d937 into master Jul 9, 2018

petermuessig deleted the feature-serveIndex branch July 9, 2018 14:56

MichaelSp mentioned this pull request Nov 20, 2020

missing files are served by index with HTTP 200 SAP/ui5-tooling#346

Closed

6 tasks

[FEATURE] simplistic serveIndex with additional properties #23

[FEATURE] simplistic serveIndex with additional properties #23

Conversation

petermuessig commented Jul 6, 2018

CLAassistant commented Jul 6, 2018 • edited Loading

coveralls commented Jul 6, 2018 • edited Loading

Pull Request Test Coverage Report for Build 102

💛 - Coveralls

petermuessig commented Jul 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RandomByte commented Jul 9, 2018

petermuessig commented Jul 9, 2018

petermuessig commented Jul 9, 2018

matz3 commented Jul 9, 2018

petermuessig commented Jul 9, 2018

RandomByte left a comment

Choose a reason for hiding this comment

RandomByte commented Jul 9, 2018 • edited by codeworrior Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Jul 6, 2018 •

edited

Loading

coveralls commented Jul 6, 2018 •

edited

Loading

RandomByte commented Jul 9, 2018 •

edited by codeworrior

Loading