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

Export plexus type declarations, remove Neutrino #348

Merged
merged 5 commits into from
Mar 19, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions .eslintignore
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
build/**/*.js
coverage/**/*.js
build
coverage
packages/plexus/src/LayoutManager/layout.worker*js*
14 changes: 7 additions & 7 deletions BUILD.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ ESLint is being used to lint the repo, as a whole. Within `./packages/plexus` (f

#### `typescript`

The TypeScript package (e.g. `typescript`) is **not for compiling** TypeScript source but is included for the purpose of bolstering the linting of TypeScript files. `tsc` catches quite a few issues that ESLint does not pick up on.
In the project root, `typescript` is used to bolster the linting of TypeScript files. `tsc` catches quite a few issues that ESLint does not pick up on.

In `./packages/plexus`, `typescript` is used to generate type declarations for the ES module build. See [`./packages/plexus/BUILD.md`](packages/plexus/BUILD.md#typescript---emitdeclarationonly) for details.

### Workspaces

Expand Down Expand Up @@ -66,7 +68,7 @@ Runs the `lint` and `test` scripts.

## `.eslintrc.js`

Pretty basic. Needs to be cleaned up. The `airbnb` configuration needs to be updated.
Pretty basic.

Note: This configuration is extended by `./packages/plexus/.eslintrc.js`.

Expand Down Expand Up @@ -96,16 +98,14 @@ A few notable [compiler settings](http://www.typescriptlang.org/docs/handbook/co
* [dom.iterable](https://github.com/Microsoft/TypeScript/blob/master/lib/lib.dom.iterable.d.ts)
* [webworker](https://github.com/Microsoft/TypeScript/blob/master/lib/lib.webworker.d.ts)
* `skipLibCheck` - Maybe worth reevaluating in the future
* `strict` - Very important
* `strict` - Important
* `noEmit` - We're using this for linting, after all
* `include` - We've included `./typgings` here because it turned out to be a lot simpler than configuring `types`, `typeRoots` and `paths`

## `typings/{custom.d.ts, index.d.ts}`

This is relevant for `./packages/plexus/src/LayoutManager/layout.worker.js` and the `viz.js` package.
This is relevant for `./packages/plexus/src/LayoutManager/getLayout.tsx` and the `viz.js` package.

I wasn't able to get much in the line of error messaging, so I'm pretty vague on this.

The version of `viz.js` in use (1.8.1) ships with an `index.d.ts` file, but it has some issues. I was able to define alternate type declarations for `viz.js` in `./typings/custom.d.ts` and referring `./typings/index.d.ts` to `./typings/custom.d.ts`. I also changed the import for `viz.js` to `viz.js/viz.js`, which is importing it's main file, directly, instead of implicitly.

For `./packages/plexus/src/LayoutManager/layout.worker.js`, webpack (in `./packages/plexus`) is set up to use the [`worker-loader`](https://github.com/webpack-contrib/worker-loader) to load the file. This converts it to a `WebWorker` by instantiating the `WebWorker` with the source as a `Blob`. Consequently, `layout.worker.js` is implemented in the context of a `WorkerGlobalScope` but consumed as if it's a regular class that extends `Worker`. To deal with this mismatch, a **webpack alias** was created mapping `worker-alias/` to `./packages/plexus/src`. This allowed a type declaration to be defined for `worker-alias/*` as a subclass of `Worker`.
The version of `viz.js` in use (1.8.1) ships with an `index.d.ts` file, but it has some issues. `./typings/custom.d.ts` defines an alternate type declaration for `viz.js` by targeting `viz.js/viz.js`. It was necessary use `./typings/index.d.ts` to refer to `./typings/custom.d.ts`. Then, importing the modules main file, which is `viz.js/viz.js`, will use the alternate type declaration.
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,9 @@
"prepare": "lerna run --stream --sort prepublishOnly",
"prettier-comment": "https://github.com/yarnpkg/yarn/issues/6300",
"prettier":
"./node_modules/prettier/bin-prettier.js --write '{.,scripts}/*.{js,json,md,ts,tsx}' 'packages/*/{src,demo/src}/**/!(react-vis).{css,js,json,md,ts,tsx}' 'packages/*/*.{css,js,json,md,ts,tsx}'",
"./node_modules/prettier/bin-prettier.js --write '{.,scripts}/*.{js,json,md,ts,tsx}' 'packages/*/{src,demo/src}/**/!(layout.worker.bundled|react-vis).{css,js,json,md,ts,tsx}' 'packages/*/*.{css,js,json,md,ts,tsx}'",
"prettier-lint":
"./node_modules/prettier/bin-prettier.js --list-different '{.,scripts}/*.{js,json,md,ts,tsx}' 'packages/*/{src,demo/src}/**/!(react-vis).{css,js,json,md,ts,tsx}' 'packages/*/*.{css,js,json,md,ts,tsx}'",
"./node_modules/prettier/bin-prettier.js --list-different '{.,scripts}/*.{js,json,md,ts,tsx}' 'packages/*/{src,demo/src}/**/!(layout.worker.bundled|react-vis).{css,js,json,md,ts,tsx}' 'packages/*/*.{css,js,json,md,ts,tsx}'",
"test": "lerna run test",
"tsc-lint": "tsc",
"tsc-lint-debug": "tsc --listFiles",
Expand Down
2 changes: 1 addition & 1 deletion packages/jaeger-ui/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
"source-map-explorer": "^1.6.0"
},
"dependencies": {
"@jaegertracing/plexus": "0.0.1-dev.4",
"@jaegertracing/plexus": "0.1.0",
"antd": "3.8.0",
"chance": "^1.0.10",
"classnames": "^2.2.5",
Expand Down
4 changes: 4 additions & 0 deletions packages/plexus/.eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,13 @@ module.exports = {
'prettier/@typescript-eslint',
],
rules: {
// use @typescript-eslint/no-useless-constructor to avoid null error on *.d.ts files
'no-useless-constructor': 0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this rule is being changed only because of *.d.ts files I believe we can define overrides, something like:

  overrides: [
    {
      files: ['*.d.ts'],
      rules: {
        'no-useless-constructor': 0,
      },
    },
  ],

Copy link
Member Author

Choose a reason for hiding this comment

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

That's for *.tsx files, as well. This override allow for the estlint-disable no-useless-constructor comments to be removed from files.


'@typescript-eslint/explicit-function-return-type': 0,
'@typescript-eslint/explicit-member-accessibility': 0,
'@typescript-eslint/no-explicit-any': 0,
'@typescript-eslint/no-useless-constructor': 1,
'@typescript-eslint/prefer-interface': 0,
},
};
4 changes: 3 additions & 1 deletion packages/plexus/.gitignore
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
build
dist
lib
src/LayoutManager/layout.worker*js*
100 changes: 0 additions & 100 deletions packages/plexus/.neutrinorc.js

This file was deleted.

99 changes: 76 additions & 23 deletions packages/plexus/BUILD.md
Original file line number Diff line number Diff line change
@@ -1,44 +1,97 @@
# Build Considerations: `./packages/plexus`

The package is implemented in TypeScript and must be compiled to JavaScript. The build output goes into `./build` as one main file, `./build/index.js`.
**Note:** File references are relative to `./packages/plexus`; `./` refers to `./packages/plexus`.

**Note:** File references are relative to `./packages/plexus`. E.g. `./` is actually referring to `./packages/plexus`, relative to the project root.
The package is implemented in TypeScript and must be compiled to JavaScript.

## Neutrino
There are three build scenarios and one pre-step common to all of them.

[Neutrino](https://master.neutrinojs.org/) is used to build `plexus`. The [release candidate for `v9`](https://github.com/neutrinojs/neutrino/milestone/6) is used.
Pre-step:

While Neutrino intends to allow for "zero initial configuration", that turned out not to be the case for `plexus`.
* Bundle `./src/LayoutManager/layout.worker.tsx` to a UMD module which can initialize a `WebWorker` from a `Blob` URL

`.neutrinorc.js` has a fair bit of configuration. It's well commented, so it won't be rehashed, here.
Build scenarios:

### `webpack.config.js`
* Production ES modules
* **This is the project's default export as `./lib/index.js`.** This build is not bundled and therefore does not use Webpack.
* Production UMD module
* Webpack dev server
* Runs `./demo/src/index.tsx` which has a few example graphs.

**Configures the webpack entry point.** Neutrino is used to generate the webpack config, but there were challenges in using it to set a non-Neutrino default entry point.
The pre-step, which they all require, is to bundle `./src/LayoutManager/layout.worker.tsx` via the `worker-loader` Webpack loader.

### `worker-alias`
## Babel

`worker-alias` is set up as a webpack alias in `.neutrinorc.js`. This alias matches a declaration in the project root: `../../typings/custom.d.ts`. See `../../BUILD.md` for details.
Babel is used to transpile the TypeScript for all scenarios and the pre-step. See `babel.config.js` for specifics.

## `package.json`
The production ES module build is not bundled and therefore does not use Webpack.

### Script `prepublishOnly`
## Webpack

Executed after `yarn install` is run in the project root.
Webpack is used to:

### Dependencies (dev and otherwise)
* Bundle `./src/LayoutManager/layout.worker.tsx` so we can have a `WebWorker` without forcing folks to deal with an additional JavaScript asset
* Bundle the production UMD module
* Run the Webpack dev server during development

#### `viz.js@1.8.1`
`./webpack-factory.js` is used to generate the Webpack configurations for each scenario.

This specific version of [viz.js](https://github.com/mdaines/viz.js) is used to avoid a regression. Meanwhile, [looks like `2.x.x`](https://github.com/mdaines/viz.js/issues/120#issuecomment-389281407) has recovered a lot of ground; [GitHub ticket](https://github.com/jaegertracing/jaeger-ui/issues/339) to upgrade.
## TypeScript `--emitDeclarationOnly`

Compiling TypeScript via Babel does not allow for type declarations to be generated. So, `tsc` is used with `./tsconfig.json` to generate the type defs.

This only applies to the ES module production build, output to `./lib`.

Note: `./tsconfig.json` does not extend `../../tsconfig.json`.

## Pre-step: `layout.worker`

#### `worker-loader`
`./src/LayoutManager/layout.worker.tsx` is intended to be loaded as a `WebWorker`. To be able to load it as a `Worker` without requiring an extra JS file, Webpack and the [`worker-loader`](https://github.com/webpack-contrib/worker-loader) loader are used to bundle it into a UMD module, `./src/LayoutManager/layout.worker.bundled.js`.

[`worker-loader`](https://github.com/webpack-contrib/worker-loader) is a webpack plugin that allows a file to be imported as a class which, when instantiated, creates a WebWorker from the underlying source.
Within the UMD module, `layout.worker.tsx` (and everything bundled into it) is turned into a `Blob` URL that's used to initialize a `WebWorker`.

#### `@babel/preset-typescript`
The resultant UMD module can be initialized as a class:

TypeScript is compiled through Babel via [`@babel/preset-typescript`](https://babeljs.io/docs/en/babel-preset-typescript). **The TypeScript compiler is not used to compile TS files.** Note: `tsc` is used to compliment the use of ESLint to lint TypeScript, though.
```ts
import LayoutWorker from './layout.worker.bundled';

const leWorker = new LayoutWorker();

leWorker.postMessage(...);
```

> It's all fun and games until type checking loses an eye.

To make sure we don't end up with an implicit `any`, `layout.worker.bundled.d.ts` provides a type declaration:

```ts
class LayoutWorker extends Worker { ... }
```

## `package.json`

### Scripts

* `build` — Generates the UMD bundle and ES module production builds
* `prepublishOnly` — Executed after `yarn install` is run in the project root; runs the `build` script
* `start` — Starts the Webpack dev server and watches all files, including `layout.worker`

The `_tasks/*` scripts are not intended to be run, directly.

* `_tasks/clean/*`
* Remove generated files
* `_tasks/bundle-worker`
* Generates the `layout.worker` UMD bundle
* `_tasks/build/*`
* Generates the production ES and UMD builds
* `_tasks/dev-server`
* Starts the Webpack dev server

### Dependencies (dev and otherwise)

#### `viz.js@1.8.1`

This specific version of [viz.js](https://github.com/mdaines/viz.js) is used to avoid a regression. Meanwhile, [looks like `2.x.x`](https://github.com/mdaines/viz.js/issues/120#issuecomment-389281407) has recovered a lot of ground; [GitHub ticket](https://github.com/jaegertracing/jaeger-ui/issues/339) to upgrade.

#### `jest@23.6.0`

Expand All @@ -48,8 +101,8 @@ Jest is not actually be used, yet. Present as a placeholder. ([Ticket](https://g

Configures ESLint for TypeScript. ESLint is executed from the project root, but this file is merged with the project root `.eslintrc.js` and overrides where there is overlap.

Uses [`@typescript-eslint/parser`](https://github.com/typescript-eslint/typescript-eslint/tree/master/packages/parser) as the parser.
`prettier/@typescript-eslint` needs to be last in the `extends` so it overrides the formatting rules from `plugin:@typescript-eslint/recommended`.

It's worth noting that the `tsconfig.json` is in the project root and `tsconfigRootDir: '.'` refers to the project root.
Uses [`@typescript-eslint/parser`](https://github.com/typescript-eslint/typescript-eslint/tree/master/packages/parser) as the parser.

`prettier/@typescript-eslint` needs to be last in the `extends` so it overrides the formatting rules from `plugin:@typescript-eslint/recommended`.
The `tsconfigRootDir: '.'` refers to the project root because that is where ESLint is executed, from. And, the `tsconfig.json` referred to by `./.eslintrc.js` is that in the project root.
53 changes: 53 additions & 0 deletions packages/plexus/babel.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
// Copyright (c) 2019 Uber Technologies, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

function getBabelConfig(api) {
const env = api.env();
return {
plugins: [
'@babel/plugin-syntax-dynamic-import',
[
'babel-plugin-transform-react-remove-prop-types',
{
removeImport: true,
},
],
[
'@babel/plugin-proposal-class-properties',
{
loose: true,
},
],
],
presets: [
[
'@babel/preset-env',
{
// this should match the settings in jaeger-ui/package.json
targets: ['>0.5%', 'not dead', 'not ie <= 11', 'not op_mini all'],
},
],
[
'@babel/preset-react',
{
development: env === 'development',
useBuiltIns: true,
},
],
'@babel/preset-typescript',
],
};
}

module.exports = getBabelConfig;
13 changes: 13 additions & 0 deletions packages/plexus/demo/template.ejs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<!DOCTYPE html>
<html lang="<%= htmlWebpackPlugin.options.lang %>">

<head>
<meta charset="utf-8">
<title><%= htmlWebpackPlugin.options.title %></title>
</head>

<body>
<div id="<%= htmlWebpackPlugin.options.appMountId %>"></div>
</body>

</html>
Loading