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

options.space in false should at least keep one for safety #11

Open
imcotton opened this issue Feb 28, 2017 · 9 comments
Open

options.space in false should at least keep one for safety #11

imcotton opened this issue Feb 28, 2017 · 9 comments

Comments

@imcotton
Copy link

Consider this edge case:

var/* jsdoc */age = 42;

Which emits:

varage = 42;
@vitaly-t
Copy link
Owner

vitaly-t commented Mar 1, 2017

That's fairly unusual way of documenting the code.

But of you run into something this strange, option space lets you replace all comment blocks with spaces.

@imcotton
Copy link
Author

imcotton commented Mar 1, 2017

I for one not seen this form as well, however in Angular v4, they're using this style all over the bundled code.

Set options.space to true has two drawbacks:

  1. It leaves quite a lot of blanks
  2. it's not default setting

@vitaly-t
Copy link
Owner

vitaly-t commented Mar 1, 2017

This must have been a recent change in how Angular is now bundled, For I used previous versions of it to test this library, and such things weren't there.

For the time being replacing comments with spaces is the only option, unfortunately, as this issue never came up before.

Later on though, I could extend option space to support an integer - the number of spaces to be used in place of each comment block, which if you set to 1 would give you exactly 1 space in place of each comment.

  1. it's not default setting

I honestly do not see it being a drawback.

@imcotton
Copy link
Author

imcotton commented Mar 1, 2017

Any chance to have fixed without using space option? Compare to uglifyjs

> echo "var/* jsdoc */age = 42" | uglifyjs 
> var age=42

@vitaly-t
Copy link
Owner

vitaly-t commented Mar 1, 2017

uglify does way more verbose JavaScript parsing, which may be an overkill for such a simple library that was designed to be small and fast.

To get an idea try replacing your example with these, see what uglify does in those cases:

echo "bla/* jsdoc */age = 42" | uglifyjs

and

echo "multi/* jsdoc

multi-line comment block

line */age = 42" | uglifyjs

@imcotton
Copy link
Author

imcotton commented Mar 1, 2017

Thinking that I can live with space number option.

I honestly do not see it being a drawback.

By drawback I meant the default setting does not as safe as verbose JavaScript parsing, rather be a performance tradeoff in this case.

@imcotton
Copy link
Author

imcotton commented Mar 1, 2017

Examples like bla/* jsdoc */age = 42 not been valid JavaScript syntax at first, but the space between declaration and identifier shall not be omitted, no?

@vitaly-t
Copy link
Owner

vitaly-t commented Mar 1, 2017

I will look at extending the space option when i get a chance 😉

vitaly-t added a commit that referenced this issue Jun 19, 2017
@vitaly-t vitaly-t added bug and removed enhancement labels Jun 19, 2017
@vitaly-t
Copy link
Owner

vitaly-t commented Jun 19, 2017

Escalating this into a bug, i.e. the library cannot process correctly special-case multi-line comments, removal of which results into code fusion:

var/*comment*/name;
// should become:
var name;
// and not:
varname;

The same with multi-line comments:

var/* multi-line
  comment */name;

Granted this is an edge case, and an odd way of adding comments, but should be supported nonetheless.

Presently it only works when using option space: true.

I have committed the tests + TODO-s that need to be implemented in the code.

A PR to fix the issue would be very welcome!

Repository owner deleted a comment Jan 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants