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

column is incorrect #269

Closed
jantimon opened this issue Aug 3, 2021 · 4 comments
Closed

column is incorrect #269

jantimon opened this issue Aug 3, 2021 · 4 comments

Comments

@jantimon
Copy link

jantimon commented Aug 3, 2021

Hi :)

I am trying to integrate stylis.js into https://astexplorer.net/ and it looks quite promising however it seems like that all column values are wrong (or I don't understand what column means).

In the following example I would expect line: 1 column: 4

    border: 1px solid black;

However it generates column 29:

AST_explorer

@Andarist
Copy link
Collaborator

Andarist commented Aug 3, 2021

I have never cared about positions in Stylis so I have never noticed that. You are right though - I don't think this can reliably be used right now.

The problem is that column is always incremented when consuming characters and nodes are only created as late as possible - which makes sense because we don't know what kind of a node this will be when we start consuming tokens for a node. So the problem is more about not keeping around the column/line values that would be useful here. Since those are values for the end position of the node in the source string - I thought that you might be able to subtract the value.length of the node from that but unfortunately no. The value might not contain all the input whitespace so the starting position cannot be reliably retrieved like that.

To be absolutely sure if this is a bug in the code of if this is by design we would have to wait for @thysultan's comment on this.

@jantimon
Copy link
Author

jantimon commented Aug 3, 2021

Thanks for your quick response

I guess we could also integrate it without positions.. In that case you might think about removing them..

Just out of curiosity - why is the column position amount to high? Couldn't we just store the column before it is incremented?
Or store the size of the current white space?

I tried removing the value and you are absolutely right - it was always wrong by 3 characters

@Andarist
Copy link
Collaborator

Andarist commented Aug 4, 2021

Just out of curiosity - why is the column position amount to high?

Because the column is always incremented when consuming tokens here:
https://github.com/thysultan/stylis.js/blob/a5acfdb536ca0c9ce2d6614b31da71dc4b8a9225/src/Tokenizer.js#L57

And it's only used when creating nodes here:
https://github.com/thysultan/stylis.js/blob/a5acfdb536ca0c9ce2d6614b31da71dc4b8a9225/src/Tokenizer.js#L20

Couldn't we just store the column before it is incremented?

This would probably be the ideal solution for a fix. The current position would have to be stored and, most likely, passed to ruleset and comment. declaration would also have to receive a value for it - but, from what I can tell, it would require a little bit more thought how to do it. The difference is that declaration doesn't consume the input stream while comment and ruleset do. So for them, it should be mostly about taking a position before they start consuming (maybe slightly adjusted by a length of some value) - declaration works differently so the strategy there would be different.

At the moment, I don't think this is overly complicated to fix but Stylis codebase is a little bit clever and it has been months since I've looked at it. Can't promise to look into it any time soon - unless you would like to take a stab at it. I would be open to answering questions about the codebase and to reviewing a PR

@thysultan
Copy link
Owner

column denotes where it ends: i.e black; at the semicolon. Rather than from the beginning border.

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

3 participants