refactor: let TypeScript do resource copying #1824

bajtos · 2018-10-08T09:47:55Z

The first commit enables TypeScript flag resolveJsonModule and leverages this feature to load datasource configurations in a type-safe way using regular "import" statements. As a result, the TypeScript compiler is taking care of copying datasource JSON files to dist now.

The second commit reorganizes test fixtures so that the build step no longer needs to copy resources to dist.

The third commit renames lb-tsc flag --ignore-resources to --copy-resources, changing the behavior of lb-tsc from "copy by default" to "copy only if asked to". This is a breaking change affecting existing LB4 projects. After this change is published, new projects are going to be scaffolded in such way that they don't need resource copying.

I have two goals in mind:

Reduce the amount of work needed to switch our monorepo to TypeScript project references (see chore: add script to build typescript project references #1636).
Speed up our build (especially in watch mode) by avoiding unnecessary build steps (like resource copying).

Checklist

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
API Documentation in code was updated
Documentation in /docs/site was updated
Affected artifact templates in packages/cli were updated
Affected example projects in examples/* were updated

raymondfeng · 2018-10-08T14:45:34Z

docs/site/Repositories.md

@@ -326,7 +326,7 @@ context.
 ```ts
 import {inject} from '@loopback/core';
 import {juggler, AnyObject} from '@loopback/repository';
-const config = require('./redis.datasource.json');
+import * as config from './redis.datasource.json';


+1 to use TS import for json files.

raymondfeng · 2018-10-08T14:50:26Z

packages/http-server/test/integration/http-server.integration.ts

@@ -211,13 +211,14 @@ describe('HttpServer (integration)', () => {
    host?: string;
  }): HttpServer {
    const options: HttpServerOptions = {protocol: 'https', host};
+    const certDir = path.resolve(__dirname, '../../../fixtures');


This is what annoys me.The directory structure is different between source code and the distribution. In this case, developers can be confused as they might assume ../../../fixtures resolve to packages/fixtures based on the source code directory chain.

IMO, we must fix this confusion in #1636 to make sure the relative path to project level files such as package.json or fixtures stays the same between source code and distribution.

raymondfeng

Assuming https://github.com/strongloop/loopback-next/pull/1824/files#r223392203 can be addressed in follow-up PRs, I approve this one.

bajtos · 2018-10-08T15:22:37Z

Cross-posting the discussion we had elsewhere.

This is what annoys me.The directory structure is different between source code and the distribution. In this case, developers can be confused as they might assume ../../../fixtures resolve to packages/fixtures based on the source code directory chain.

Yes, I agree this is annoying.

IMO, we must fix this confusion in #1636 to make sure the relative path to project level files such as package.json or fixtures stays the same between source code and distribution.

I still would like to give a try to the alternative ts-project-layout approach I have described in #1636 (comment) - let src and test be compiled independently. In that case, src goes to dist, test goes to something like dist-test and relative paths remain the same in both source and dist.

If we decide to follow your approach where __tests__ are nested under src, the problem goes away too, because src/__tests__/foo.js is transpiled to dist/__tests__/foo.js and thus the relative paths remain the same.

So either way we choose, the problem of confusing relative paths can be addressed as part of project-references refactor.

Enable TypeScript feature "resolveJsonModule" to load datasource configurations in a type-safe way using regular "import" statements.

Move test fixtures from `/test` to package root directories. This way the build step does not have to copy them over to `/dist`, which saves time and will make the watch mode more effective once we enable it.

Rework `lb-tsc` to NOT copy resources by default. Drop `--ignore-resources` flag (which implied that resources are copied by default). Add a new flag `--copy-resources` to allow old projects to request the old behavior.

marioestradarosa · 2018-10-08T15:55:07Z

👍 for If we decide to follow your approach where __tests__ are nested under src, the problem goes away too,

bajtos self-assigned this Oct 8, 2018

bajtos requested a review from raymondfeng October 8, 2018 09:47

bajtos added the breaking-change label Oct 8, 2018

bajtos changed the title ~~Refactor: let TypeScript do resource copying~~ refactor: let TypeScript do resource copying Oct 8, 2018

bajtos requested review from marioestradarosa and a team October 8, 2018 09:59

bajtos force-pushed the refactor/remove-resource-copy branch from c354efd to d15857c Compare October 8, 2018 10:03

bajtos mentioned this pull request Oct 8, 2018

chore: add script to build typescript project references #1636

Closed

7 tasks

raymondfeng reviewed Oct 8, 2018

View reviewed changes

raymondfeng approved these changes Oct 8, 2018

View reviewed changes

bajtos added 3 commits October 8, 2018 17:22

feat: use resolveJsonModule to load datasource config

009861a

Enable TypeScript feature "resolveJsonModule" to load datasource configurations in a type-safe way using regular "import" statements.

refactor: reorganize test fixtures to avoid need for resource copying

1b8cfe5

Move test fixtures from `/test` to package root directories. This way the build step does not have to copy them over to `/dist`, which saves time and will make the watch mode more effective once we enable it.

feat(build): rename --ignore-resources to --copy-resources

034151f

Rework `lb-tsc` to NOT copy resources by default. Drop `--ignore-resources` flag (which implied that resources are copied by default). Add a new flag `--copy-resources` to allow old projects to request the old behavior.

bajtos force-pushed the refactor/remove-resource-copy branch from d15857c to 034151f Compare October 8, 2018 15:22

bajtos merged commit 2958ace into master Oct 8, 2018

bajtos deleted the refactor/remove-resource-copy branch October 8, 2018 15:44

bajtos mentioned this pull request Oct 9, 2018

feat(boot): bind content of package.json to app context #1764

Merged

7 tasks

bajtos added the Internal Tooling Issues related to our tooling and monorepo infrastructore label Mar 18, 2019

bajtos mentioned this pull request Mar 18, 2019

Enable TypeScript Project References #2609

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: let TypeScript do resource copying #1824

refactor: let TypeScript do resource copying #1824

bajtos commented Oct 8, 2018 •

edited

Loading

raymondfeng Oct 8, 2018

raymondfeng Oct 8, 2018 •

edited

Loading

raymondfeng left a comment

bajtos commented Oct 8, 2018 •

edited

Loading

marioestradarosa commented Oct 8, 2018

refactor: let TypeScript do resource copying #1824

refactor: let TypeScript do resource copying #1824

Conversation

bajtos commented Oct 8, 2018 • edited Loading

Checklist

raymondfeng Oct 8, 2018

Choose a reason for hiding this comment

raymondfeng Oct 8, 2018 • edited Loading

Choose a reason for hiding this comment

raymondfeng left a comment

Choose a reason for hiding this comment

bajtos commented Oct 8, 2018 • edited Loading

marioestradarosa commented Oct 8, 2018

bajtos commented Oct 8, 2018 •

edited

Loading

raymondfeng Oct 8, 2018 •

edited

Loading

bajtos commented Oct 8, 2018 •

edited

Loading