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

[FEATURE] Add JSDoc build functionalities #42

Merged
merged 38 commits into from
Mar 21, 2019

Conversation

codeworrior
Copy link
Member

Note: the code is full of console.log just to track down an unexpected exit of the Node.js process.

@codeworrior codeworrior requested a review from RandomByte June 28, 2018 12:22
@RandomByte
Copy link
Member

RandomByte commented Jun 28, 2018

By writing a resource, the FileSystem adapter drains its content stream. Therefore the .clone workaround makes sense. However, I'll look into a way to have the FS adapter to provide a new stream.

There is this undocumented contract that whoever reads from a Resources stream has to provide a new readable stream via setStream

@RandomByte
Copy link
Member

Basically, the issue is as follows:
tmp fs write of resources

Writing resource A to the file system requires to drain the stream. Currently, after that operation the Resource has irreversibly lost any content, making it impossible to further process this resource (content is somewhat essential).

I see two possible solutions here:

  1. Before the write operation, load the content of A into a buffer.
    => After the write operation, content A₂ is still available
    Only downside is the potentially high memory consumption caused by all those buffers.
  2. The resource gets a new readable stream of the file that has been written to FS (tmp)
    => After the write operation, content A₂ is still available
    However, any "physical" changes done to that file will be applied to resource A too. This might be unexpected.

@RandomByte
Copy link
Member

I think 1. is closer to the expected behavior, I'll try to prepare a ui5-fs PR for that soon.

@RandomByte
Copy link
Member

Started working on the discussed ui5-fs fix in SAP/ui5-fs#22

@RandomByte
Copy link
Member

RandomByte commented Jan 31, 2019

Blocker should be resolved with SAP/ui5-fs#22

@RandomByte RandomByte force-pushed the create-jsdoc-for-libraries branch from 26f25fd to 996c711 Compare February 11, 2019 14:01
@CLAassistant
Copy link

CLAassistant commented Feb 11, 2019

CLA assistant check
All committers have signed the CLA.

@RandomByte
Copy link
Member

Rebased

@RandomByte RandomByte force-pushed the create-jsdoc-for-libraries branch from 996c711 to 92e32ef Compare February 26, 2019 13:16
@RandomByte
Copy link
Member

RandomByte commented Feb 26, 2019

Implementation Concept

JSDoc generation UI5 Tooling integration.pdf

Additional Implementation Details

  • Preferably add a "ui5 build jsdoc” command defining all JSDoc related tasks as enabled by default
  • Add ui5.yaml configuration for JSDoc excludes (like thirdparty- or very big, not relevant resources). Grunt JSDoc build currently does not have any exclude handling. Maven JSDoc build reads this information from .library
  • Enhance Custom Task implementation for defining "final" tasks (lower part of the chart) and positioning as “first/last” task as there might not be any active "final" task that could be used for relative positioning OBSOLETE - Tasks that where meant to be executed in this "final" stage shall be moved into an "SDK" project that, when being built, will be executed last anyways
  • Processor returns array of resources but also writes to fs? (yes, at least for jsdoc config, but maybe different location?) (maybe different dest location?) => Only writes to tmp fs. Currently easiest solution to have easy reuse of UI5 JSDoc libs and proper separation of concerns

CC: @tommyvinhlam

@RandomByte RandomByte force-pushed the create-jsdoc-for-libraries branch from 92e32ef to e7680f3 Compare February 26, 2019 14:19
@RandomByte RandomByte force-pushed the create-jsdoc-for-libraries branch 2 times, most recently from 2489edd to 7ac62c7 Compare February 28, 2019 15:16
@devtomtom devtomtom self-requested a review March 1, 2019 07:52
@RandomByte RandomByte force-pushed the create-jsdoc-for-libraries branch from 7ac62c7 to 399af06 Compare March 1, 2019 11:48
@RandomByte
Copy link
Member

RandomByte commented Mar 5, 2019

Not sure anymore whether the project name should be included in the temporary path directly. At least we should remove all alpha-num characters to prevent directory traversal

Edit: Done with 0d4a77f

@RandomByte
Copy link
Member

b93d4bd depends on SAP/ui5-fs#112

@RandomByte
Copy link
Member

RandomByte commented Mar 7, 2019

TODO: Document new JSDoc exclude configuration in UI5 Project - Done: SAP/ui5-project#152

Example from sap.ui.core:

---
specVersion: "0.1"
type: library
metadata:
  name: sap.ui.core
  copyright: |-
   OpenUI5
    * (c) Copyright 2009-${currentYear} SAP SE or an SAP affiliate company.
    * Licensed under the Apache License, Version 2.0 - see LICENSE.txt.
builder:
  jsdoc:
    excludes:
    - "jquery-*"
    - "sap-ui-*"
    - "sap/ui/debug/**"
    - "sap/ui/qunit/**"
    - "sap/ui/thirdparty/**"

@RandomByte
Copy link
Member

Still not sure whether JSDoc exclude patterns should include the virtual base path (/resources / /test-resources) to allow differentiation between the two (or more). Currently they are designed to only work within /resources (just like the JSDoc build itself).

This came to my mind when thinking about the definition of project-wide exclude patterns as requested in SAP/ui5-tooling#124. They should probably include the virtual paths to allow excluding resources of /test-resources for example.

So even though the JSDoc build itself is only working on /resources, having the same flavor of include/exclude patterns (that include the virtual base path) in the ui5.yaml might make sense.

@matz3
Copy link
Member

matz3 commented Mar 8, 2019

@RandomByte I would align this with the bundler, which also doesn't contain resources or test-resources in the pattern/filters but implicitly uses resources.

@RandomByte
Copy link
Member

We had another discussion and tend to keep JSDoc exclude patterns aligned with the bundler patterns while use absolute virtual paths (/resources/pony/**) for more general excludes.

@RandomByte RandomByte force-pushed the create-jsdoc-for-libraries branch 2 times, most recently from 2707f80 to 5718c87 Compare March 11, 2019 13:20
…on or .library resources are found

Globs may return resources in random order, therefore choosing the first
of many matches can lead to erratic results.
This prevents JSDoc errors in case no resources have written to the
source path.
@RandomByte RandomByte force-pushed the create-jsdoc-for-libraries branch from d91ea6e to 8272acb Compare March 18, 2019 15:10
@RandomByte
Copy link
Member

Rebased

@RandomByte RandomByte force-pushed the create-jsdoc-for-libraries branch from ed3cb45 to 652d0cd Compare March 19, 2019 14:48
devtomtom
devtomtom previously approved these changes Mar 20, 2019
Copy link
Member

@devtomtom devtomtom left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@codeworrior
Copy link
Member Author

I've tried it out on OpenUI5 (together with the pending change there) and it worked quite well!

As css files are required for a working SDK
They are not necessary in preview/dev scenarios
@RandomByte RandomByte requested a review from devtomtom March 21, 2019 14:56
Copy link
Member

@devtomtom devtomtom left a comment

Choose a reason for hiding this comment

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

👍

@RandomByte RandomByte merged commit 293a4b0 into master Mar 21, 2019
@RandomByte RandomByte deleted the create-jsdoc-for-libraries branch March 21, 2019 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants