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

dist contains whole typescript lib #642

Open
ywwhack opened this issue Jan 12, 2021 · 12 comments
Open

dist contains whole typescript lib #642

ywwhack opened this issue Jan 12, 2021 · 12 comments

Comments

@ywwhack
Copy link

ywwhack commented Jan 12, 2021

When I use ncc build, the output contains all typescript/lib, such as below:
image

I try to comment a require statement like require(someConfigPath),it then no longer output typescript/lib. It seems like import a module using "require" has some problems.

Is there any workaround to use "require" and the dist dir not contains the whole typescript/lib ?

@simenandre
Copy link

I have the same problem. Is there a way to ignore the package?

@songkeys
Copy link
Contributor

append -e typescript

@simenandre
Copy link

@songkeys This excludes typescript but from what I understand, it's because one of @ywwhack packages or the code depends on typescript. This in turns means that we'll get Error: Cannot find module 'typescript'

@songkeys
Copy link
Contributor

songkeys commented Feb 2, 2021

@cobraz I think the production environment should not need any help from the typescript lib. Why would you need type checking / generating in the production env? You can avoid requiring it with a conditional requiring if you are requiring but not actually using it:

const typescript = process.env.NODE_ENV === 'production' ? {} : require('typescript')

If one of your dependencies is requiring typescript, you can have a dirty patch on the generated code:

let content = fs.readFileSync('index.js', "utf-8");
content = content.replace(/require\("typescript"\)/g, "{}")
fs.writeFileSync('index.js', content)

I assume this dependency is also requiring it but not using it in the production env.

@simenandre
Copy link

simenandre commented Feb 2, 2021

@songkeys You can see specifically what it is used for in pulumi/action#50. It's actually a Github Actions I'm trying to compile into one file. Commiting 400+ Typescript files feels wrong, and actually, the underlying package @pulumi/pulumi uses typescript in the runtime parts. (e.g. https://github.com/pulumi/pulumi/blob/master/sdk/nodejs/runtime/closure/utils.ts).

I don't know yet if I actually need typescript for anything, or if I can dirty patch it. We'll see :)

If you have any other ideas/approaches; I'd love some feedback!

@ghost
Copy link

ghost commented Feb 3, 2021

Funny enough, I currently face the opposite problem: I do need typescript to be included in the file generated by ncc in order to transpile some user-written TS-code, but whenever I try to use ncc on even the simplest script such as

    import * as ts from 'typescript';
    eval(ts.transpile('const x: number = 5; console.log(x);'));

I get the following error:
ncc: Compiling file typescript/lib/tsserver.js
Error: Conflict: Multiple assets emit different content to the same filename typescript/lib/tsserver.js
at C:\Users\abc\AppData\Roaming\npm\node_modules@vercel\ncc\dist\ncc/index.js.cache.js:1:3258429
at C:\Users\abc\AppData\Roaming\npm\node_modules@vercel\ncc\dist\ncc/index.js.cache.js:1:2210953
at eval (eval at create (C:\Users\abc\AppData\Roaming\npm\node_modules@vercel\ncc\dist\ncc/index.js.cache.js:1:1140143), :13:1)

nwtgck added a commit to nwtgck/actions-netlify that referenced this issue May 4, 2021
nwtgck added a commit to nwtgck/actions-netlify that referenced this issue May 4, 2021
@EvanBoyle
Copy link

If one of your dependencies is requiring typescript, you can have a dirty patch on the generated code:

@songkeys would it be possible to enable this via an ncc build flag? I think this would be similar to webpack's exclude: https://webpack.js.org/configuration/module/#ruleexclude

Then rather than applying a patch, a similar outcome could be achieved with a single command ncc build index.ts -e typescript --exclude typescript

@songkeys
Copy link
Contributor

songkeys commented Jun 3, 2021

would it be possible to enable this via an ncc build flag?

@EvanBoyle It would be much convenient if we have one. I believe many unused dependencies (like typescript here) are still require-ing on top level of a file in production environment, which may cause errors because we've --excluded them when bundling.

But I'm not sure if such a flag is generally reasonable and applicable. But you can patch it yourself anyway.

@guybedford @styfle What do you think? Should we provide something like ncc build index.ts --drop typescript to completely remove a package by applying a patch as follows?

let content = fs.readFileSync('index.js', "utf-8");
content = content.replace(/require\("typescript"\)/g, "{}")
fs.writeFileSync('index.js', content)

@styfle
Copy link
Member

styfle commented Jun 3, 2021

You can't replace like that.

Imagine existing code:

const typescript = require('typescript')
const result = typescript.compiler.transform('file.ts')

If you replaced it with your solution then it would fail at runtime because there would be missing properties:

const typescript = {}
const result = typescript.compiler.transform('file.ts')

The correct solution is to use ncc build index.js -e typescript or the longer form --external as mentioned above.

See the docs here: https://github.com/vercel/ncc#options

@songkeys
Copy link
Contributor

songkeys commented Jun 3, 2021

If you replaced it with your solution then it would fail at runtime because there would be missing properties

I knew. It works only when we are certain that it won't break our production code. I.e. we are sure that the code containing missing properties will never run in production environment.

But yes. It's not generally a correct solution.

@EvanBoyle
Copy link

The correct solution is to use ncc build index.js -e typescript or the longer form --external as mentioned above.

@styfle this leaves a top level require("typescript") in the code. that statement alone fails at runtime. For our app we know that:

  1. typescript is not required
  2. removing the `require("typescript") with the patch here does not cause a runtime failure

I would describe it as "manual tree shaking". As a user I know that this dependency is not needed for the specific app I am building, but the compiler won't drop it, and the exclude flag doesn't help because the top level require gets left in the resulting code.

I understand that I can accomplish this with a patch on the generated code, but this another manual step I have to maintain. Seems very similar to https://webpack.js.org/configuration/module/#ruleexclude which has first class support in webpack.

I also understand that this flag would not be conservatively correct, and would rely on judgement of users to use appropriately.

@styfle
Copy link
Member

styfle commented Jun 4, 2021

ncc is built on webpack so feel free to submit a PR with a test if you want to add this feature 👍

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

5 participants