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

.ts files being added to the end build #156

Closed
j opened this issue Dec 15, 2018 · 8 comments
Closed

.ts files being added to the end build #156

j opened this issue Dec 15, 2018 · 8 comments

Comments

@j
Copy link

j commented Dec 15, 2018

Is it on purpose that .ts files are also included in the end build?

@macklinu
Copy link
Contributor

I can't reproduce this with v0.5.5 with the following example:

index.ts

import chalk from 'chalk'
import log from './log'

log(chalk.green('Hello World'))

log.ts

export default function log(message: string) {
  console.log(message)
}

package.json

{
  "scripts": {
    "dev": "ncc run index.ts",
    "build": "ncc build index.ts -o dist"
  },
  "dependencies": {
    "chalk": "^2.4.1"
  },
  "devDependencies": {
    "@zeit/ncc": "0.5.5"
  }
}

Output of yarn build:

yarn run v1.12.3
$ ncc build index.ts -o dist
23kB  dist/index.js
23kB  [2823ms]
Done in 3.01s.

The only output I see is dist/index.js and dist/index.js.map.

@j Do you have a reproducible example that could be used to help resolve your issue?

@j
Copy link
Author

j commented Dec 17, 2018

Interesting, I'm going to try to reproduce it. On a simple project, it's not doing it. Here's my output right now:

dist
 0kB  dist/common/interfaces/index.ts
 0kB  dist/common/interfaces/CartDocument.ts
 0kB  dist/common/interfaces/Document.ts
 0kB  dist/common/interfaces/BundleItemDocument.ts
 1kB  dist/common/saveToS3.ts
 1kB  dist/common/interfaces/BundleDocument.ts
 1kB  dist/common/sns/index.ts
 1kB  dist/common/config.ts
 1kB  dist/common/db/MongoProvider.ts
 2kB  dist/common/interfaces/Event.ts
 2kB  dist/common/screenshotConfig.ts
 3kB  dist/common/Logger.ts
39kB  dist/index.js
51kB  [12163ms]

I'll try to reproduce it. Something is triggering it :-\

@j
Copy link
Author

j commented Dec 17, 2018

Somehow it's copying over EVERYTHING in my common folder, and I can't reproduce it.......

@macklinu
Copy link
Contributor

@j would you be able to share your tsconfig.json if you have one locally? Wonder if that could be part of it?

@j
Copy link
Author

j commented Dec 17, 2018

@macklinu I figured it out.

Output:

> ncc build src/services/foo/handler.ts -o dist

0kB  dist/common/interfaces/Foo.ts
0kB  dist/common/config.ts
0kB  dist/common/log.ts
3kB  dist/index.js
3kB  [1934ms]

src/index.ts

import { APIGatewayProxyEvent, APIGatewayProxyResult } from "aws-lambda";
import { FOO } from "./common/config";

export const foo = async (
  _event: APIGatewayProxyEvent
): Promise<APIGatewayProxyResult> => {
  return {
    statusCode: 200,
    body: FOO
  };
};

src/common/config.ts

import { join } from "path";
import { config } from "dotenv";

const root = (file: string): string => {
  return join(__dirname, "..", "..", file);
};

config(root(".env"));

export const FOO = process.env.FOO;

You can put anything in src/common that isn't require(*)'d anywhere and it will get included in the distribution (notice interfaces/Foo.ts and log.ts are included but not required)

{
  "compilerOptions": {
    "target": "es2015",
    "moduleResolution": "node"
  }
}

So where this is happening is the line:

join(__dirname, "..", "..", file);

I simplified this example (mine uses dynamic .env files). If I remove the dynamic aspect and hardcode the join to:

config(join(__dirname, "..", "..", ".env"));

then, common/* is not being included.

Interesting...

@j
Copy link
Author

j commented Dec 17, 2018

Looking at the relocate-loader, looks like a lot is going on with "join" guestimation. Could be a scary thing for some (perhaps I have some privates being exposed)

@guybedford
Copy link
Contributor

In order to get lots of Node packages to work, given the limited execution analysis power we have, yes the emission does overshoot more than it needs to to be safe, but this is under the security assumption that if "pkg/" is safe to publish then so is it to emit any assets from there.

In terms of tying down the security here, one thing we could add is to ensure that no package emits assets below the base-level folder of that package itself (the package.json folder).

This would break some emission cases, but in the name of a much more comprehensible security model which definitely does seem important.

Another thing to add for this issue specifically might be a filter or ignore folders option to specifically say when not to emit assets from certain locations.

@guybedford
Copy link
Contributor

In the latest work in asset analysis in #378, a new restriction has been put in place that wildcard directory assets are never emitted for folders that backtrack below the module doing the emission.

In addition node_modules packages cannot emit assets outside of their own package folders, node_modules will never be emitted, and no assets will be emitted below the cwd of the build itself.

I believe that should resolve a lot of these incorrect emission issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants