Skip to content
This repository has been archived by the owner on Jul 9, 2021. It is now read-only.

Improved Docs & Removed monorepo-scripts from packages #924

Merged
merged 195 commits into from
Aug 23, 2018

Conversation

fabioberger
Copy link
Contributor

@fabioberger fabioberger commented Jul 29, 2018

Description

Currently monorepo-scripts is a dependency of every public package. The reason? We are using Lerna's postpublish.js script hook to generate/upload docs and publish release notes to Github. This PR changes that.

As the number of packages published in the repo has grown, it has become untenable to have an individual release note for each package. This PR therefore aggregates them into a single release note that looks like:

### 0x.js

- Fixed a tiny, inconsequential bug (#789)
- Add a `findALover` method to `ZeroEx` (#793)

### web3-wrapper

- Removed unused param from `getBlockAsync` (#794)

...and so on for each package

Additionally the doc generation/upload step has been moved to the publish.ts script and happens after the packages are published. Importantly, the way we generate the doc JSON representations has changed. It now does the following:

  • Find all exports in packages index.ts
  • Find the files for each export (if it's internal or within another monorepo package)
  • Call Typedoc with list of files containing the public interface of the package
  • Go through the resulting Typedoc JSON, and remove all exported elements not exported in the main package's index.ts.
  • Add entries for any external packages re-exported by our package (previously the docs simply omitted these).

These steps have resulted in our doc JSON files being 3X smaller then they used to be! 🍾 🎆

Additionally, we've gotten rid of a lot of package specific configurations using the above procedure. This PR also adds checks to make sure that any types that are present in the package's public interface are exported by it's index.ts file. It will even complain if the package exports superfluous type declarations. These checks now happen in every CircleCI run.

In order to make all package's pass these new constraints, I had to modify their index.ts. Some larger changes were also needed, like moving types from sol-compiler to types that are used by many packages now.

Miscelaneous fixes:

  • Run yarn install twice in CircleCI since I was encountering the same issue as locally.
  • Modified the public interface of 0x.js so that it simply re-exports it's sub-packages.
  • Added OrderWatcher back into 0x.js
  • Fixed a bunch of TSLint errors from across many unchanged packages that suddenly popped up.
  • Fixed scroll offset issue

Testing instructions

Run the tests. Render the doc pages.

Types of changes

Checklist:

  • Prefixed the title of this PR with [WIP] if it is a work in progress.
  • Prefixed the title of this PR with bracketed package name(s) corresponding to the changed package(s). For example: [sol-cov] Fixed bug.
  • Added tests to cover my changes, or decided that tests would be too impractical.
  • Updated documentation, or decided that no doc change is needed.
  • Added new entries to the relevant CHANGELOG.jsons.

…ggregate release note publishing to publish script
@albrow
Copy link
Contributor

albrow commented Aug 22, 2018

@fabioberger 👍 sounds good to me. I'll take any chance we can get to reduce maintenance burden right now.

Copy link
Contributor

@fragosti fragosti left a comment

Choose a reason for hiding this comment

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

This is amazing but also a lot. Really hard to get some confidence that there aren't any bugs. Overall looks good -- I'm down to ship and then discover what bugs there are since it doesn't touch critical pieces toooo much. Must've been a lot of work so 🙏🏼

@@ -82,7 +74,7 @@
"nyc": "^11.0.1",
"shx": "^0.2.2",
"tslint": "5.11.0",
"typedoc": "~0.8.0",
"typedoc": "0xProject/typedoc",
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this just so that we are using the same version everywhere. There used to be a bug in Typedoc that was causing us grief. Refactors since then have made it a non-issue so I changed all versions to the latest one of 0.12.0.

@@ -1,9 +1,9 @@
// tslint:disable:no-consecutive-blank-lines ordered-imports align trailing-comma whitespace class-name
// tslint:disable:no-unused-variable
// tslint:disable:no-unbound-method
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you suddenly need to add these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I upgraded our tslint-eslint-rules dependency since it's dep tsutils@^1.4.0 wasn't playing nicely with TS 3.0. I believe this rule was added to it.

);
});
} else if (_.isObject(nodeValue)) {
updatedReferenceNames = updatedReferenceNames = DocGenerateAndUploadUtils._getAllReferenceNames(
Copy link
Contributor

Choose a reason for hiding this comment

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

Woah this compiles?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yikes, good catch.

});
if (!_.isEmpty(externalExportsWithoutLinks)) {
throw new Error(
`Found the following external exports in ${
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice.

const changelogs = JSON.parse(changelogJSON);
const latestLog = changelogs[0];
// If only has a `Dependencies updated` changelog, we don't include it in release notes
if (latestLog.changes.length === 1 && latestLog.changes[0].note === constants.dependenciesUpdatedMessage) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make this more explicit and put a shouldOmitFromReleaseNotes boolean in CHANGELOG.json?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, the thing is that dev's don't add the Dependencies updated entry in CHANGELOG.json, it's automatically added if no entry was manually added to the CHANGELOG.

@@ -48,7 +48,7 @@ export const utils = {
};
packages.push(pkg);
} catch (err) {
utils.log(`Couldn't find a 'package.json' for ${subpackageName}. Skipping.`);
// Couldn't find a 'package.json' for package. Skipping.
Copy link
Contributor

Choose a reason for hiding this comment

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

Too noisy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, too noisy and very unlikely to be an issue.

@@ -1,84 +0,0 @@
/* Basscss Responsive Type Scale */
/* Modified by Fabio Berger to include xs prefix */
Copy link
Contributor

Choose a reason for hiding this comment

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

Why delete all these files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when I created the react-docs-example package, we had just separated out the docs rendering code from the website and were under the impression that others would want to use it. This didn't pan out. Additionally, I just don't feel confident given this most recent refactor that this package is ready for prime-time. We can always add it back once it is, but right now, I'd rather not give the impression that it's ready.

@@ -13,7 +13,7 @@
"TRADITIONAL_ASSETS": "传统资产",
"DIGITAL_GOODS": "数字商品",
"OFFCHAIN_ORDER_RELAY": "链下订单中继",
"OONCHAIN_SETTLEMENT": "链上最终结算",
Copy link
Contributor

Choose a reason for hiding this comment

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

Was wondering about this lol

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lol, silly typo.

@fabioberger fabioberger merged commit 6e27324 into development Aug 23, 2018
@fabioberger fabioberger deleted the wrap-typedoc branch August 23, 2018 17:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants