Skip to content
This repository has been archived by the owner on Jan 21, 2022. It is now read-only.

lerna build #117

Merged
merged 16 commits into from
Feb 22, 2019
Merged

lerna build #117

merged 16 commits into from
Feb 22, 2019

Conversation

evgeniuz
Copy link
Contributor

Fixes #107

@CLAassistant
Copy link

CLAassistant commented Nov 14, 2018

CLA assistant check
All committers have signed the CLA.

@evgeniuz
Copy link
Contributor Author

evgeniuz commented Nov 14, 2018

Added very rough lerna build, still lots to do, but packages are split and building. Instructions are:

yarn install
lerna bootstrap
lerna run build:ts
node package/ethql/dist/index.js

Things still to do (non-complete list):

  • move tests into packages and ensure they're working
  • move package lifecycle scripts into packages
  • move dependencies into packages
  • fix docker build
  • fix circle CI
  • update README

@evgeniuz
Copy link
Contributor Author

evgeniuz commented Nov 22, 2018

Implemented lerna build in the best way possible (I hope 😃):

  • It uses new TypeScript feature (build mode and project references), this allows to run complete build and watching across all packages at once (also replaced nodemon with tsc-watch, as it's much simpler wrapper around tsc and more suitable to work in build mode).
  • It uses "global" tsconfig.json which is not used for actual build, but used for jest. This allows to run one instance of jest to execute all tests across packages (including watch mode and coverage mode).

This combination allows running tests or dev/debug build in watch mode for easier development and is producing loosely coupled dist folders for distribution.

Instruction:

yarn install
lerna bootstrap
...
# it's not required to build before running dev or test
yarn build:ts
yarn dev
yarn test
etc.

Things I still plan to do:

  • fix docker build
  • fix circle CI
  • update README with new setup instructions

Except for these things it's ready for some review (just to confirm that approach taken is good one).

@evgeniuz
Copy link
Contributor Author

Fixed docker build, but not sure what's with circleCI.

@evgeniuz evgeniuz changed the title WIP: lerna build lerna build Nov 22, 2018
@evgeniuz
Copy link
Contributor Author

Technically everything should be complete, but CircleCI tests are timing out for some reason (work locally and don't take too much time). Cannot troubleshoot this as output is truncated.

@evgeniuz
Copy link
Contributor Author

Cause for failure was that it was requesting too many workers and correspondingly allocating too much memory. Fixed by limiting workers to 2.

@raulk
Copy link
Contributor

raulk commented Dec 2, 2018

@evgeniuz thanks for the submission! I fixed the jest configuration to honor the worker limitation (not sure where this came from), and updated all versions. Could you please integrate these changes on your branch?

Copy link
Contributor

@raulk raulk left a comment

Choose a reason for hiding this comment

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

A few initial comments.

@@ -8,6 +8,8 @@ export type EthqlServiceDefinition<Config, Service> = {
};
};

export interface EthqlServiceDefinitions {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this needed?

Copy link
Contributor Author

@evgeniuz evgeniuz Dec 2, 2018

Choose a reason for hiding this comment

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

This interface is extended in every service but had no "empty" definition. This caused problem compiling @ethql/base as there is just one service and it's not imported anywhere in itself (it's imported in ethql package, but since we need to be able to build @ethql/base in isolation, this is not relevant here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To add: this empty definition makes sense here as it should be object where keys are services, and without any services defined it would be an empty object (that later can be extended).

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

import resolvers from './resolvers';

import {} from '@ethql/base/dist/core/services/web3';
Copy link
Contributor

Choose a reason for hiding this comment

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

It's incorrect to import compiled javascript from a Typescript file.

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 think it’s correct way to approach this: if you intend to publish all packages on NPM, there will be no TS sources in packages, only compiled files. So if we refer to TS files, these imports will be unresolved when installed from NPM.

No types information is lost, as all those compiled files always have accompanying .d.ts with them.

Ideally, base package should export single index.ts file that will contain all other exports in it to remove paths and just import @ethql/base everywhere, but I think this should be done after this PR is integrated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And tsc build mode intended to work with this approach: it checks if referenced compiled import is present and if not present it builds it from TS.

import resolvers from './resolvers';

import {} from '@ethql/base/dist/core/services/web3';
import {} from './services/ens';
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I get this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change has similar cause to EthqlServiceDefinitions change. plugin: EthqlPluginFactory has references to both ens service and web3 service, but those are not imported at this point, so TS complains that there are no web3 EthqlServiceDefinition defined. So we need to import this file here. Basically, we need to import file that defines service if we depend on it.

packages/ens/src/resolvers/index.ts Outdated Show resolved Hide resolved
@evgeniuz
Copy link
Contributor Author

evgeniuz commented Dec 7, 2018

@raulk Rebased on top of recent master changes.

@evgeniuz
Copy link
Contributor Author

Are there any changes I need to implement here?

@raulk raulk changed the base branch from master to lerna January 27, 2019 14:06
@raulk
Copy link
Contributor

raulk commented Jan 27, 2019

I pushed a commit a few days ago performing some further refactor, but didn't have time to comment on it. Here it goes:

  • Teased out the core plugin from the base module. This was pretty traumatic because it required quite a bit of disentanglement, but the result is much cleaner.
  • Extracted the web3 typings to a module of its own.
  • Added main and typings fields in package.json files, and added index.ts files where they were missing, making importing more idiomatic.

@raulk
Copy link
Contributor

raulk commented Jan 27, 2019

Just pushed a few more commits to rationalise the build commands; hopefully it'll now pass on CI. I also fixed the Dockerfile.

@kshinn kshinn merged commit 90efc55 into Consensys:lerna Feb 22, 2019
@gitcoinbot gitcoinbot mentioned this pull request May 7, 2019
9 tasks
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