[Monorepo] Adopt @glimmer/component and @glimmer/application #3

tomdale · 2017-08-15T21:32:34Z

This PR begins the process of turning this repo into a monorepo that contains "application-level" Glimmer packages (as opposed to "VM-level" packages in glimmer-vm).

To start, we import @glimmer/component and @glimmer/application. These are imported using the script in script/import-packages.sh which preserves each repository's git history.

Features

Lerna for managing dependencies

Each package can have its own dependencies; these are installed by running lerna bootstrap.

Single Root `tsconfig.json`

This prevents VS Code from getting confused when one package imports another across tsconfig.json boundaries, which may have different settings. The single tsconfig.json keeps VS Code happy and also keeps code quality consistent across subpackages.

Use `paths` for package resolution

We use tsconfig.json's paths setting to allow TypeScript to find local packages, even when imported via package name. This is preferable to symlinking into node_modules because relying on Node module resolution causes TypeScript to look at package.json, which points at .d.ts files instead of the actual TypeScript source. The implication of this is that when you command-click a symbol from another package in VS Code, you get taken to the actual source code instead of a generated .d.ts file, and cross-package changes are reflected in the editor immediately without waiting for a build step.

Unified build

Build and test all packages in the repository via a single Broccoli build script. The build infrastructure is very similar to the recent work for glimmer-vm with some tweaks:

Work with existing repository layout, so we can import without major changes
Run tests in ES6+ instead of transpiling ES5 as the VM does since we have less stringent compatibility constraints and this allows us to do things like use async functions in tests.

Multi-root API

Initialize `_rootsIndex` with a value

Improved tests

Set `args` on components

With this commit, tests import everything from the package's root `index.ts` file. This facilitates testing production builds of the package, where internal modules are hoisted and thus lost.

Monorepo preparation

- rm files from correct directory - fix glob for gitignore - yarn install and lerna bootstrap after import

dgeb

Looks good @tomdale!

rwjblue

Need a build script (likely also as prepublish).

rwjblue · 2017-08-21T20:56:52Z

.travis.yml

+cache: yarn
+
+addons:
+  firefox: "54.0"


We should update to use chrome with --headless instead (should be a follow up PR I believe).

rwjblue · 2017-08-21T20:58:31Z

bin/publish.js

+}
+
+// Fail fast if we haven't done a build first.
+assertDistExists();


FWIW, I think we should just always ember build -prod here instead of checking that dist exists. This same script exists in glimmer-vm and I have published old builds at least 5 times 🤦‍♂️ .

Ugh, sorry about that. I updated the script to eagerly perform a build always.

rwjblue · 2017-08-21T20:59:31Z

bin/publish.js

+}
+
+function assertGitIsClean() {
+  let status = execSync('git status').toString();


Should use --porcelain option (otherwise users git configs break this)

TIL, thank you

rwjblue · 2017-08-21T21:04:12Z

build/broccoli/build-tests.js

+  });
+
+  let glimmerVM = funnel('packages', {
+    include: ['@glimmer/*/node_modules/@glimmer/*/dist/amd/es5/*.js']


I don't fully grok what is happening here. What creates packages/@glimmer/<whatever>/node_modules/@glimmer/compiler? What prevents duplicates from being concatted?

Packages are installed here by Lerna during lerna bootstrap. Duplicates are indeed concatted, which is allowed by our AMD loader so long as it duplicates are registered before the module is reified. This is obviously not ideal, but requires a mechanism for having per-package dependencies in the tests. I wanted to get something working landed and then iterate on resolving that problem.

Sounds good, mind creating an issue and linking to it from a comment? Just to make sure we don't lose track of the concern...

rwjblue · 2017-08-21T21:04:50Z

build/broccoli/build-tests.js

+    destDir: '/assets'
+  });
+
+  let qunit = funnel(path.join(require.resolve('qunitjs'), '..'), {


Is this going to end up watching all of node_modules?

No, because:

> require.resolve('qunitjs') '/Users/tomdale/Code/glimmer.js/node_modules/qunitjs/qunit/qunit.js'

So this resolves to node_modules/qunitjs/qunit.

Awesome, thank you for confirming.

rwjblue · 2017-08-21T21:10:06Z

package.json

@@ -0,0 +1,50 @@
+{
+   "name": "glimmer.js",
+   "version": "0.8.0",


Seems odd to bump this here (others are on 0.7.x I believe)

I was thinking we should synchronize versions here like we do with glimmer-vm since @glimmer/application and @glimmer/component are so coupled. They're on 0.7.2 and 0.6.0 respectively now so I think we'd need to bump to 0.8.0 if we want to sync versions.

Totally agreed that we should synchronize, but what I was actually getting at was that the "bumping" of the version number should be done all at the same time with the publish script (the last time I ran the bin/publish in glimmer-vm where one package had a different version number I got an error)

rwjblue · 2017-08-21T21:15:12Z

build/symlink-dependencies/index.js

+  name: 'symlink-dependencies',
+
+  preBuild() {
+    rimraf('dist/@glimmer/*/node_modules');


This seems pretty weird: clearing dist/ when we may not even be building into dist/

Is line 29 the best way to determine target output directory for the current build?

Hmm, not really (it doesn't account for ember build --output-path=./some-path) but I don't think we can do better here without changes upstream.

I'd suggest we add a process.env.EMBER_CLI_BUILD_OUTPUT_PATH environment variable (or expose it via some specific API that addons have access to).

rwjblue · 2017-08-21T21:17:45Z

build/symlink-dependencies/index.js

@@ -0,0 +1,66 @@
+'use strict';


This addon seems unused? Did you intend to add ember-addon: { paths: ['./build/symlink-dependencies']} to the package.json?

chadhietala · 2017-08-30T20:46:34Z

I'm going to block as there is a cycle.

Update
This dependency should not be an issue as the actual runtime code it just referring to interfaces and the tests are using concrete implementations. This seems "ok" for now.

⠋ BuildingWarning: cycle detected: @glimmer/component <- @glimmer/application <- @glimmer/component

Registry access refactor

Move Environment, DynamicScope, and Iterable modules to glimmer-application

tomdale and others added 30 commits March 23, 2017 14:25

0.3.3

1d6cb1a

Bump dependencies

69059a6

0.3.3

3e9f277

Bump dependencies

0713e42

0.3.4

13ca333

Improve typing of roots property

324dc54

Minor refactor

b639271

New API for rendering roots

97b8ffb

Precompile root template

882d563

Super small refactor

8d4524e

Improved template handling

2ab07c6

Fix a type on renderComponent

9c4967e

Set args on components

ea2ecf4

Merge pull request #25 from robbiepitts/roots

3aa7b98

Multi-root API

Add publish hooks

99a5d8c

0.3.4

a55dc51

Use state bucket instead of WeakMap

09017fd

Fix incorrect component definition cache key

45dc916

0.3.5

109b035

Initialize _rootsIndex with a value

78d098d

Merge pull request #26 from robbiepitts/roots-index

a29438e

Initialize `_rootsIndex` with a value

Trim whitespace from built-in template

33386cb

More realistic tests

40b6ca5

Merge pull request #27 from glimmerjs/improved-tests

c6b50fd

Improved tests

0.3.6

76efff3

Multi-roots compatibility

50aebb8

Bump @glimmer/build

c86434c

Remove simple-html-tokenizer

58c89d4

Merge pull request #17 from glimmerjs/component-args

2993c22

Set `args` on components

0.3.5

7867bbf

tomdale added 16 commits August 14, 2017 16:09

Import package exports from root package

ec5971b

With this commit, tests import everything from the package's root `index.ts` file. This facilitates testing production builds of the package, where internal modules are hoisted and thus lost.

Import package exports from root package

dac1a3b

With this commit, tests import everything from the package's root `index.ts` file. This facilitates testing production builds of the package, where internal modules are hoisted and thus lost.

Sync tslint.json config from glimmer-vm

a6e4bcc

Merge pull request #74 from glimmerjs/pre-monorepo

9b2b1ff

Monorepo preparation

Merge pull request #96 from glimmerjs/pre-monorepo

eafbfb8

Monorepo preparation

Monorepo infrastructure

e4ca5ea

Import packages/@glimmer/component

af063c1

Import packages/@glimmer/application

32e5184

Update packages for monorepo

0eab1c1

Fix Simple DOM type issue

88cf06c

Update package dependencies and remove scripts

2b65c0c

Enable Travis

ef6ab75

Fix import script

a300027

- rm files from correct directory - fix glob for gitignore - yarn install and lerna bootstrap after import

Add test and postinstall scripts for root package.json

679ff62

Update Firefox version

08350c7

Enable Travis yarn cache

e74eefb

dgeb self-requested a review August 17, 2017 16:08

dgeb approved these changes Aug 17, 2017

View reviewed changes

tomdale mentioned this pull request Aug 20, 2017

tracked property (dependent on "args") in child component is out of sync glimmerjs/glimmer-component#66

Closed

rwjblue reviewed Aug 21, 2017

View reviewed changes

chadhietala added 2 commits August 30, 2017 15:35

Import packages/@glimmer/application\-test\-helpers

f969c28

Pull in test helpers

b702105

Share test helpers

c86a98d

chadhietala mentioned this pull request Aug 30, 2017

Issue for tracking per-package dependencies in tests #5

Closed

chadhietala merged commit 4fe4bad into master Aug 31, 2017

chadhietala pushed a commit that referenced this pull request Aug 31, 2017

Merge pull request #3 from glimmerjs/registrations

76edda4

Registry access refactor

chadhietala pushed a commit that referenced this pull request Aug 31, 2017

Merge pull request #3 from glimmerjs/move-environment

d2cb516

Move Environment, DynamicScope, and Iterable modules to glimmer-application

Turbo87 added the internal label Oct 17, 2017

locks deleted the monorepo-infrastructure branch October 18, 2017 08:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Monorepo] Adopt @glimmer/component and @glimmer/application #3

[Monorepo] Adopt @glimmer/component and @glimmer/application #3

tomdale commented Aug 15, 2017

dgeb left a comment

rwjblue left a comment

rwjblue Aug 21, 2017

tomdale Aug 23, 2017

rwjblue Aug 21, 2017

tomdale Aug 23, 2017

rwjblue Aug 21, 2017

tomdale Aug 23, 2017

rwjblue Aug 21, 2017

tomdale Aug 23, 2017

rwjblue Aug 23, 2017

rwjblue Aug 21, 2017

tomdale Aug 23, 2017

rwjblue Aug 23, 2017

rwjblue Aug 21, 2017

tomdale Aug 23, 2017

rwjblue Aug 23, 2017

rwjblue Aug 21, 2017

tomdale Aug 23, 2017

rwjblue Aug 23, 2017

rwjblue Aug 21, 2017

chadhietala commented Aug 30, 2017 •

edited

Loading

[Monorepo] Adopt @glimmer/component and @glimmer/application #3

[Monorepo] Adopt @glimmer/component and @glimmer/application #3

Conversation

tomdale commented Aug 15, 2017

Features

Lerna for managing dependencies

Single Root tsconfig.json

Use paths for package resolution

Unified build

dgeb left a comment

Choose a reason for hiding this comment

rwjblue left a 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

chadhietala commented Aug 30, 2017 • edited Loading

Single Root `tsconfig.json`

Use `paths` for package resolution

chadhietala commented Aug 30, 2017 •

edited

Loading