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

Compile in two steps, tsc + webpack #8

Closed
wants to merge 1 commit into from

Conversation

simark
Copy link
Contributor

@simark simark commented Nov 13, 2018

Instead of compiling the typescript and webpacking in one go, do the
process in two steps:

  • Compile ts files to corresponding js files
  • Webpack the js files

I find it useful to see the intermediary .js files and be able to
inspect them. Also, debugging the webpack bundle does not work
currently. Debugging the intermediary js files work. We'll of couse
want to be able to debug the bundle, but this gives us a way to do it in
the mean time.

A functional change of this patch is that the bundle is now generated
to dist/bundle.js, which is a kind of de facto standard place for it.
it can be renamed to anything when distributed.

Signed-off-by: Simon Marchi simon.marchi@ericsson.com

Instead of compiling the typescript and webpacking in one go, do the
process in two steps:

- Compile ts files to corresponding js files
- Webpack the js files

I find it useful to see the intermediary .js files and be able to
inspect them.  Also, debugging the webpack bundle does not work
currently.  Debugging the intermediary js files work.  We'll of couse
want to be able to debug the bundle, but this gives us a way to do it in
the mean time.

A functional change of this patch is that the bundle is now generated
to dist/bundle.js, which is a kind of de facto standard place for it.
it can be renamed to anything when distributed.

Signed-off-by: Simon Marchi <simon.marchi@ericsson.com>
@eclipse-cdt-bot
Copy link
Contributor

Can one of the admins verify this patch?

@jonahgraham
Copy link
Contributor

ok to test

@dschaefer
Copy link
Contributor

ok to test

Need to add you as an admin :). Feel free to tweak the job to add in this phrase. For now I have it set as

run tests

@dschaefer
Copy link
Contributor

Not sure the benefit of running both tsc and webpack. ts-loader essentially calls tsc. And, really, all the adapter is going to be is TypeScript so webpack has nothing else to do. I would think you'd use one or the other, not both.

I like shrinking this down to a single file for production. That's a pretty standard practice with npm packages. If you want to just call tsc in development and drop webpack, I could live with that.

@simark
Copy link
Contributor Author

simark commented Nov 13, 2018

Not sure the benefit of running both tsc and webpack. ts-loader essentially calls tsc. And, really, all the adapter is going to be is TypeScript so webpack has nothing else to do. I would think you'd use one or the other, not both.

I have a slight preference for running the tools separately. I find it's easier to understand the pipeline, and it's possible to run each step independently if needed. Like if you are trying to debug a webpack issue, you just run webpack in isolation, there is less stuff invovled. It just offers more flexibility than a single tool that does everything. And there is no significant impact on the build time.

I like shrinking this down to a single file for production. That's a pretty standard practice with npm packages. If you want to just call tsc in development and drop webpack, I could live with that.

I completely agree with packing everything to a single file for production. I just think that having access to intermediary files can be interesting, for example when you need to dig in what the typescript compiler did.

@simark simark mentioned this pull request Nov 13, 2018
@dschaefer
Copy link
Contributor

We've simplified this in #11

@dschaefer dschaefer closed this Nov 13, 2018
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.

None yet

4 participants