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

Improve developer debug experience by allowing individual JS files to be served in an app #46

Closed
andyberry88 opened this Issue Oct 7, 2013 · 15 comments

Comments

Projects
None yet
6 participants
@andyberry88
Member

andyberry88 commented Oct 7, 2013

JavaScript (and other) files are always bundled. This makes debugging JavaScript very difficult:

  1. Very large files make finding code difficult
  2. The files are so large that browser debuggers can't find the source of exceptions and keep track of breakpoints

This also impacts #7.

Technical task:
Update js bundler servlet to serve files individually in dev by default, but with a configuration switch in app.conf to turn it back to the old way

comment from @andyberry88
'can we have a command line switch so this can be done easily as a one off? e.g. in CI i might want a bundle but in dev single files.'

see https://github.com/BladeRunnerJS/brjs-private/issues/170 for original issue

@ctrlbreak

This comment has been minimized.

Show comment
Hide comment
@ctrlbreak

ctrlbreak Oct 7, 2013

I'm not sure that individual in dev is actually what we want. It makes the page load much slower, even from localhost. I thought we talked about a batching strategy so it would collect them together into multiple merged JS bundles, but just smaller than one huge impossible to debug monolith?

ctrlbreak commented Oct 7, 2013

I'm not sure that individual in dev is actually what we want. It makes the page load much slower, even from localhost. I thought we talked about a batching strategy so it would collect them together into multiple merged JS bundles, but just smaller than one huge impossible to debug monolith?

@leggetter

This comment has been minimized.

Show comment
Hide comment
@leggetter

leggetter Oct 7, 2013

Contributor

@kybernetikos can provide more information on this as he has spiked this. I believe individual files works quite well.

Contributor

leggetter commented Oct 7, 2013

@kybernetikos can provide more information on this as he has spiked this. I believe individual files works quite well.

@andyberry88

This comment has been minimized.

Show comment
Hide comment
@andyberry88

andyberry88 Oct 7, 2013

Member

We need to do this to fix the issue of line numbers not matching up - which makes debugging painfully tedious. I think we should have a command line switch to do this rather than making it the default, so its easier to debug but the default way of working is to use what is deployed in production.

Member

andyberry88 commented Oct 7, 2013

We need to do this to fix the issue of line numbers not matching up - which makes debugging painfully tedious. I think we should have a command line switch to do this rather than making it the default, so its easier to debug but the default way of working is to use what is deployed in production.

@andyberry88

This comment has been minimized.

Show comment
Hide comment
@andyberry88

andyberry88 Oct 8, 2013

Member

@dchambers comment on old issue:
"I don't think there should be a switch to change modes since this doubles
the required testing effort each time you fix a bug -- e.g. test it once in
mode A, and then again in mode B. I think that we should always bundle in a
single blob if there are any Caplin style classes or we are exporting to
flat files, whereas we should bundle files individually if all classes are
wrapped in closures and we are in dev."

Member

andyberry88 commented Oct 8, 2013

@dchambers comment on old issue:
"I don't think there should be a switch to change modes since this doubles
the required testing effort each time you fix a bug -- e.g. test it once in
mode A, and then again in mode B. I think that we should always bundle in a
single blob if there are any Caplin style classes or we are exporting to
flat files, whereas we should bundle files individually if all classes are
wrapped in closures and we are in dev."

@andyberry88

This comment has been minimized.

Show comment
Hide comment
@andyberry88

andyberry88 Oct 8, 2013

Member

Theres still 2 mode there that I need to test in, we've then got the concept of dev and prod mode leaching back in again. Maybe we need a call/design discussion before we make any progress on this.

Member

andyberry88 commented Oct 8, 2013

Theres still 2 mode there that I need to test in, we've then got the concept of dev and prod mode leaching back in again. Maybe we need a call/design discussion before we make any progress on this.

@leggetter

This comment has been minimized.

Show comment
Hide comment
@leggetter

leggetter Oct 8, 2013

Contributor

The debug experience is fundamentally broken right now. If browsers can't cope with our bundle files (they're too big to get the line numbers correct) then we simply must work around this. At the moment individual files seems to be the simplest approach.

Contributor

leggetter commented Oct 8, 2013

The debug experience is fundamentally broken right now. If browsers can't cope with our bundle files (they're too big to get the line numbers correct) then we simply must work around this. At the moment individual files seems to be the simplest approach.

@stevesouth

This comment has been minimized.

Show comment
Hide comment
@stevesouth

stevesouth Oct 8, 2013

Contributor

I think there should be a switch and it should be in the js bundle tag (not via the command line). Whether you have a dev / prod issue is then up to you as a developer, but we should support both mechanisms. We could guide everyone down the right path by leaving 'bundled' as the default.

Contributor

stevesouth commented Oct 8, 2013

I think there should be a switch and it should be in the js bundle tag (not via the command line). Whether you have a dev / prod issue is then up to you as a developer, but we should support both mechanisms. We could guide everyone down the right path by leaving 'bundled' as the default.

@dchambers

This comment has been minimized.

Show comment
Hide comment
@dchambers

dchambers Oct 8, 2013

Contributor

Individulal files isn't the simplest approach if it means your app works
differently in dev and prod. If you are using CommonJS style then this is
fine, and we can switch without affecting functionality, but if you are
using CaplinJS style then switching changes the hoisting, and may change
the observed functionality.

The debug experience for CaplinJS isn't fundamentally broken either, and
browsers can handle it just fine. It's just a little slower, and the line
numbers correlate to the bundle, which is available on disk. Not perfect,
but okay, and an incentive to upgrade to CommonJS style.

On Tue, Oct 8, 2013 at 9:07 AM, Phil Leggetter notifications@github.comwrote:

The debug experience is fundamentally broken right now. If browsers can't
cope with our bundle files (they're too big to get the line numbers
correct) then we simply must work around this. At the moment individual
files seems to be the simplest approach.


Reply to this email directly or view it on GitHubhttps://github.com/BladeRunnerJS/brjs/issues/46#issuecomment-25871505
.

Contributor

dchambers commented Oct 8, 2013

Individulal files isn't the simplest approach if it means your app works
differently in dev and prod. If you are using CommonJS style then this is
fine, and we can switch without affecting functionality, but if you are
using CaplinJS style then switching changes the hoisting, and may change
the observed functionality.

The debug experience for CaplinJS isn't fundamentally broken either, and
browsers can handle it just fine. It's just a little slower, and the line
numbers correlate to the bundle, which is available on disk. Not perfect,
but okay, and an incentive to upgrade to CommonJS style.

On Tue, Oct 8, 2013 at 9:07 AM, Phil Leggetter notifications@github.comwrote:

The debug experience is fundamentally broken right now. If browsers can't
cope with our bundle files (they're too big to get the line numbers
correct) then we simply must work around this. At the moment individual
files seems to be the simplest approach.


Reply to this email directly or view it on GitHubhttps://github.com/BladeRunnerJS/brjs/issues/46#issuecomment-25871505
.

@andyberry88

This comment has been minimized.

Show comment
Hide comment
@andyberry88

andyberry88 Oct 8, 2013

Member

@leggetter Yep, that I 100% agree with. What I don't like is that individual files becomes the default way of working, and we loose the power of dev and prod being the same.

To be 100% sure that it works as I expect I still have to check bundled and non-bundled versions. Also, making it a config option means that in CI I'm using the same config as in dev, un-bundled. So the first time I find out theres something wrong in my app due to bundling is when I deploy it, possibly 2 weeks after I wrote the bit of code thats broken.

A switch suggests that this isn't the default way of working, the default is bundling. If devs want to debug, or just want to always run with un-bundled mode then they can always add the switch before running the server. But at least its then a conscious decision and people are more aware that this isn't how it happens in prod.

Member

andyberry88 commented Oct 8, 2013

@leggetter Yep, that I 100% agree with. What I don't like is that individual files becomes the default way of working, and we loose the power of dev and prod being the same.

To be 100% sure that it works as I expect I still have to check bundled and non-bundled versions. Also, making it a config option means that in CI I'm using the same config as in dev, un-bundled. So the first time I find out theres something wrong in my app due to bundling is when I deploy it, possibly 2 weeks after I wrote the bit of code thats broken.

A switch suggests that this isn't the default way of working, the default is bundling. If devs want to debug, or just want to always run with un-bundled mode then they can always add the switch before running the server. But at least its then a conscious decision and people are more aware that this isn't how it happens in prod.

@ctrlbreak

This comment has been minimized.

Show comment
Hide comment
@ctrlbreak

ctrlbreak Oct 8, 2013

I agree this is not as simple an issue as it sounds. And I DO like the fundamental approach of "dev == production". I hate modes, and it's a big selling point of BRJS.

But we do need to facilitate easier debugging as well. I understand that source maps have been ruled out for some reason too?

So it needs a bit more thought before we implement it. I was talking to James about it yesterday - we should set up a session.

ctrlbreak commented Oct 8, 2013

I agree this is not as simple an issue as it sounds. And I DO like the fundamental approach of "dev == production". I hate modes, and it's a big selling point of BRJS.

But we do need to facilitate easier debugging as well. I understand that source maps have been ruled out for some reason too?

So it needs a bit more thought before we implement it. I was talking to James about it yesterday - we should set up a session.

@dchambers

This comment has been minimized.

Show comment
Hide comment
@dchambers

dchambers Oct 8, 2013

Contributor

When Adam Iley looked into this he observed three potential differences
between bundling and individually serving files:

  1. Missing semi colons: didn't affect CommonJS style code, and a
    semi-colon could be automatically inserted after each class so it didn't
    affect CaplinJS style code either.
  2. Runtime errors: didn't affect CommonJS style code.
  3. Hoisting: didn't affect CommonJS style code.

So, to be clear, if all of your classes are using CommonJS style, then
there can be no difference between serving the code bundled or
individually. If, on the other hand, you do have some CaplinJS style
classes, then you are likely to observe differences, and if you allow the
code to be served in different ways in dev and prod then you are removing
the "works the same in dev as prod" advantage.

There may be some benefit in some of these suggestions about allowing the
developer to change how the app works (either using the tag or on the
command line), but I think we need to understand what these advantages are,
and think carefully about any switching mechanisms we offer (if we offer
any), since we should be careful to maintain the "works the same in dev as
prod" advantage if at all possible.

Unless somebody can explain to me any advantages for serving individual
files in prod, or bundled files in dev, then I am of the opinion that no
switch is necessary for BladeRunnerJS developers that will be using the
CommonJS style. As for CaplinTrader developers that may have the old style,
then perhaps a switch makes more sense, or maybe some migration tools to
allow conversion to a style where each class is wrapped in a closure,
whether that be to CommonJS style, or to some intermediarry CaplinJS+ style.

-- Dom

On Tue, Oct 8, 2013 at 9:13 AM, Andy Berry notifications@github.com wrote:

@leggetter https://github.com/leggetter Yep, that I 100% agree with.
What I don't like is that individual files becomes the default way of
working, and we loose the power of dev and prod being the same.

To be 100% sure that it works as I expect I still have to check bundled
and non-bundled versions. Also, making it a config option means that in CI
I'm using the same config as in dev, un-bundled. So the first time I find
out theres something wrong in my app due to bundling is when I deploy it,
possibly 2 weeks after I wrote the bit of code thats broken.

A switch suggests that this isn't the default way of working, the default
is bundling. If devs want to debug, or just want to always run with
un-bundled mode then they can always add the switch before running the
server. But at least its then a conscious decision and people are more
aware that this isn't how it happens in prod.


Reply to this email directly or view it on GitHubhttps://github.com/BladeRunnerJS/brjs/issues/46#issuecomment-25871825
.

Contributor

dchambers commented Oct 8, 2013

When Adam Iley looked into this he observed three potential differences
between bundling and individually serving files:

  1. Missing semi colons: didn't affect CommonJS style code, and a
    semi-colon could be automatically inserted after each class so it didn't
    affect CaplinJS style code either.
  2. Runtime errors: didn't affect CommonJS style code.
  3. Hoisting: didn't affect CommonJS style code.

So, to be clear, if all of your classes are using CommonJS style, then
there can be no difference between serving the code bundled or
individually. If, on the other hand, you do have some CaplinJS style
classes, then you are likely to observe differences, and if you allow the
code to be served in different ways in dev and prod then you are removing
the "works the same in dev as prod" advantage.

There may be some benefit in some of these suggestions about allowing the
developer to change how the app works (either using the tag or on the
command line), but I think we need to understand what these advantages are,
and think carefully about any switching mechanisms we offer (if we offer
any), since we should be careful to maintain the "works the same in dev as
prod" advantage if at all possible.

Unless somebody can explain to me any advantages for serving individual
files in prod, or bundled files in dev, then I am of the opinion that no
switch is necessary for BladeRunnerJS developers that will be using the
CommonJS style. As for CaplinTrader developers that may have the old style,
then perhaps a switch makes more sense, or maybe some migration tools to
allow conversion to a style where each class is wrapped in a closure,
whether that be to CommonJS style, or to some intermediarry CaplinJS+ style.

-- Dom

On Tue, Oct 8, 2013 at 9:13 AM, Andy Berry notifications@github.com wrote:

@leggetter https://github.com/leggetter Yep, that I 100% agree with.
What I don't like is that individual files becomes the default way of
working, and we loose the power of dev and prod being the same.

To be 100% sure that it works as I expect I still have to check bundled
and non-bundled versions. Also, making it a config option means that in CI
I'm using the same config as in dev, un-bundled. So the first time I find
out theres something wrong in my app due to bundling is when I deploy it,
possibly 2 weeks after I wrote the bit of code thats broken.

A switch suggests that this isn't the default way of working, the default
is bundling. If devs want to debug, or just want to always run with
un-bundled mode then they can always add the switch before running the
server. But at least its then a conscious decision and people are more
aware that this isn't how it happens in prod.


Reply to this email directly or view it on GitHubhttps://github.com/BladeRunnerJS/brjs/issues/46#issuecomment-25871825
.

@ctrlbreak

This comment has been minimized.

Show comment
Hide comment
@ctrlbreak

ctrlbreak Oct 8, 2013

Individual is noticeably slower to refresh the page. And it IS actually different: despite what you say about semantic/parsing differences, we don't know how every browser will treat files.

I think there's a bigger picture here though. Once we add obfuscation (already supposed to be there!) or more minification, plus allow users to write plugins that modify bundles or files, we need to think about whether/how to allow devs to run on the unmodified source vs modified vs bundled etc. I'd love to make it simple, but still give the dev = production capability (whether by default/switch/whatever)

I don't think we can do more on this over email - we need to get together to discuss.

ctrlbreak commented Oct 8, 2013

Individual is noticeably slower to refresh the page. And it IS actually different: despite what you say about semantic/parsing differences, we don't know how every browser will treat files.

I think there's a bigger picture here though. Once we add obfuscation (already supposed to be there!) or more minification, plus allow users to write plugins that modify bundles or files, we need to think about whether/how to allow devs to run on the unmodified source vs modified vs bundled etc. I'd love to make it simple, but still give the dev = production capability (whether by default/switch/whatever)

I don't think we can do more on this over email - we need to get together to discuss.

@dchambers

This comment has been minimized.

Show comment
Hide comment
@dchambers

dchambers Oct 8, 2013

Contributor

We definitely need a meeting, but a precursor it would really help to see a
test case where bundled and individual served files are handled different
(given that we will be using an automatically added closure which won't
have an error in it) for any of the browsers we support. This would
definitely help inform our decision.

On Tue, Oct 8, 2013 at 9:49 AM, ctrlbreak notifications@github.com wrote:

Individual is noticeably slower to refresh the page. And it IS actually
different: despite what you say about semantic/parsing differences, we
don't know how every browser will treat files.

I think there's a bigger picture here though. Once we add obfuscation
(already supposed to be there!) or more minification, plus allow users to
write plugins that modify bundles or files, we need to think about
whether/how to allow devs to run on the unmodified source vs modified vs
bundled etc. I'd love to make it simple, but still give the dev =
production capability (whether by default/switch/whatever)

I don't think we can do more on this over email - we need to get together
to discuss.


Reply to this email directly or view it on GitHubhttps://github.com/BladeRunnerJS/brjs/issues/46#issuecomment-25873603
.

Contributor

dchambers commented Oct 8, 2013

We definitely need a meeting, but a precursor it would really help to see a
test case where bundled and individual served files are handled different
(given that we will be using an automatically added closure which won't
have an error in it) for any of the browsers we support. This would
definitely help inform our decision.

On Tue, Oct 8, 2013 at 9:49 AM, ctrlbreak notifications@github.com wrote:

Individual is noticeably slower to refresh the page. And it IS actually
different: despite what you say about semantic/parsing differences, we
don't know how every browser will treat files.

I think there's a bigger picture here though. Once we add obfuscation
(already supposed to be there!) or more minification, plus allow users to
write plugins that modify bundles or files, we need to think about
whether/how to allow devs to run on the unmodified source vs modified vs
bundled etc. I'd love to make it simple, but still give the dev =
production capability (whether by default/switch/whatever)

I don't think we can do more on this over email - we need to get together
to discuss.


Reply to this email directly or view it on GitHubhttps://github.com/BladeRunnerJS/brjs/issues/46#issuecomment-25873603
.

@sospirited

This comment has been minimized.

Show comment
Hide comment
@sospirited

sospirited Nov 19, 2013

Member

Which branch has this work been done in? I don't see any pull requests nor any commits which relate specifically to this issue

Member

sospirited commented Nov 19, 2013

Which branch has this work been done in? I don't see any pull requests nor any commits which relate specifically to this issue

@andyberry88

This comment has been minimized.

Show comment
Hide comment
@andyberry88

andyberry88 Nov 20, 2013

Member

The work is on the bundler-exploration branch and hasn't yet been merged into develop.

Member

andyberry88 commented Nov 20, 2013

The work is on the bundler-exploration branch and hasn't yet been merged into develop.

@andyberry88 andyberry88 removed the 5 - Done label Jun 18, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment