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

feat(cli): Use swc for deno bundle #7461

Closed
wants to merge 15 commits into from
Closed

Conversation

kdy1
Copy link

@kdy1 kdy1 commented Sep 14, 2020

To ease testing, I added codes to cli/tsc.rs.
I'll move codes to relevant module after some testing.


Output of deno bundle https://deno.land/[email protected]/http/server.ts: https://gist.github.com/kdy1/fee62b5f2e6dcc4639e2f16277978ccd

Timings are:

target/debug/deno bundle 'https://deno.land/[email protected]/http/server.ts' 0.24s user 0.02s system 96% cpu 0.266 total
target/release/deno bundle 'https://deno.land/[email protected]/http/server.ts' 0.03s user 0.01s system 80% cpu 0.052 total


I found another bug while testing it on other std modules. Fix seems simple, so I'll fix it asap.

Fixed with swc-project/swc#1083.


I found a bug of `swc_bundler`, and working on it at https://github.com/swc-project/swc/pull/1075 (Circular import bug)

Sadly I found some more bugs.

@CLAassistant
Copy link

CLAassistant commented Sep 14, 2020

CLA assistant check
All committers have signed the CLA.

@kdy1
Copy link
Author

kdy1 commented Sep 18, 2020

It works well. I tested http/server.ts, async/mod.ts, examples/cat.ts, example/colors.ts from 0.69.0 (using deno.land url).

(Command: cargo run -- bundle https://deno.land/[email protected]/examples/colors.ts > colors.js)

But I didn't write tests yet because I have to look for file fetcher and module graph.

@kdy1 kdy1 changed the title [WIP] feat(cli): Use swc to bundle modules feat(cli): Use swc for deno bundle Sep 18, 2020
@bartlomieju
Copy link
Member

CC @kitsonk could you take a look?

@kdy1 kdy1 marked this pull request as ready for review September 18, 2020 10:38
@ry ry added this to the 1.5.0 milestone Sep 21, 2020
@kitsonk
Copy link
Contributor

kitsonk commented Sep 23, 2020

Finally have gotten some time to look at this.

I rebased it off master, and took it for a spin. I am getting the following with this:

> deno bundle https://deno.land/x/[email protected]/mod.ts oak.js
thread 'main' panicked at 'internal error: entered unreachable code: failed to calculate least common ancestors of [ModuleId(1), ModuleId(39)]', /Users/kitsonk/.cargo/registry/src/github.com-1ecc6299db9ec823/swc_bundler-0.7.4/src/bundler/chunk/plan/lca.rs:36:9

I suspect it is related to the fact that most of the code is on https://deno.land/ but there is one module in the dependency tree that is on https://raw.githubusercontent.com/.

This also seems to suffer from #3802:

> deno bundle https://deno.land/x/oak/mod.ts oak.js 
Bundle https://deno.land/x/oak/mod.ts
Emit "oak.js" (0B)

There are other tests I would like to do to see how much of #4549 this would fix or make unneeded to address, but a few thoughts/observations:

  • It would still be super beneficial to support source-maps, but also load them from code.
  • To make bundles super useful, we need to investigate .d.ts generation, and placing the /// <reference types="..." /> in the bundle.
  • This is implicitly --no-check. We need to re-work things so that it is a check by default (but uses this to do the emit).
  • There are some finer points of import.meta that we need to figure out (most of which are mentioned in Bundle v3).

My overall opinion is that I would love to take this PR and fold it into the compiler rewrite. Specifically adapt this as the next PR after #7621.

@kdy1
Copy link
Author

kdy1 commented Sep 23, 2020

I was actually thinking it might be better to make it as an off-by-default flag.

@kdy1 kdy1 marked this pull request as draft September 23, 2020 03:29
@kitsonk
Copy link
Contributor

kitsonk commented Sep 23, 2020

I was actually thinking it might be better to make it as an off-by-default flag.

Possibly, but that is a significant change in behaviour. It was discussed in #5436 and we agreed to type check by default. My personal opinion is that we wouldn't want to change that behaviour until Deno 2 at the earliest, even then, with the concept of a "secure" development tool, when using TypeScript, I would think people would expect it to type check by default.

@ry
Copy link
Member

ry commented Sep 23, 2020

IMO tree shaking seems more important than type checking when bundling.

@kdy1
Copy link
Author

kdy1 commented Sep 23, 2020

@kitsonk I meant an option like --no-check (to use swc_bundler), which is off by default.
I'm bit worried about it as swc_bundler is not mature enough.
There are many test for crazy cases, but well... real word codes are quite divergent

@kitsonk
Copy link
Contributor

kitsonk commented Sep 23, 2020

IMO tree shaking seems more important than type checking when bundling.

There is no reason we can have both, and from a time perspective, it will be no worse than what we have today.

@kitsonk
Copy link
Contributor

kitsonk commented Sep 23, 2020

Ah, using swc_bundler for --no-check would be a good first step, certianly... we don't have --no-check support for deno bundle yet, but I think with a bit of work, we could get comfortable with it being the main mechanism for bundling.

@kdy1
Copy link
Author

kdy1 commented Sep 24, 2020

To make bundles super useful, we need to investigate .d.ts generation, and placing the /// in the bundle.

I recently made lots of progress. Type inference, including type parameter inference is almost done. Many type inference tests from official tsc passes, but I think more tests are required.

See: swc-project/swc#571
Repo: https://github.com/swc-project/typescript


Edit: After some debugging, I found it's related to circular dependencies.
I'm working on it at swc-project/swc#1105

@kdy1
Copy link
Author

kdy1 commented Oct 1, 2020

Closing in favor of #7669

@kdy1 kdy1 closed this Oct 1, 2020
@kdy1 kdy1 deleted the swc-bundler branch October 1, 2020 04:48
@kdy1 kdy1 restored the swc-bundler branch October 1, 2020 04:48
@kdy1 kdy1 deleted the swc-bundler branch October 1, 2020 04:48
@kdy1 kdy1 restored the swc-bundler branch October 1, 2020 04:49
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

Successfully merging this pull request may close these issues.

5 participants