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

Fixup JSON AST conversion #1488

Merged
merged 7 commits into from Dec 9, 2020
Merged

Fixup JSON AST conversion #1488

merged 7 commits into from Dec 9, 2020

Conversation

mischnic
Copy link
Contributor

@mischnic mischnic commented Dec 8, 2020

I've found two problems related to Input:

1std and 2nd commit: I missed that all nodes have a source property, not just the root (this caused stringification to fail)

3rd commit: More of an optimization: because of 1), the source contents & map would be stored once for every node (!). This has something like a quadratic memory usage because larger CSS files have more files, so with 10x more rules, the CSS source is about 10x larger and is stored 10x times.
I've instead deduplicated these inputs objects by replacing node.source.input with a number in the toJSON output and including a _inputs array which maps number to input object (because most of these are ===-equal). This is then reversed in fromJSON. This breaks a buch of postcss-parser-tests tests at the moment though. What do you think of this approach? (Before I try to adjust the parser test runner to accept this version...)

lib/node.js Show resolved Hide resolved
lib/fromJSON.js Outdated Show resolved Hide resolved
lib/node.js Outdated Show resolved Hide resolved
@ai
Copy link
Member

ai commented Dec 9, 2020

postcss-parser-tests/jsonify.js is the best place to fix tests

@mischnic
Copy link
Contributor Author

mischnic commented Dec 9, 2020

postcss-parser-tests/jsonify.js is the best place to fix tests

Should that function also delete the inputId and inputs properties?

@ai
Copy link
Member

ai commented Dec 9, 2020

Should that function also delete the inputId and inputs properties?

Yeap. Send PR. I will release it.

@ai
Copy link
Member

ai commented Dec 9, 2020

Are we ready to release it?

@mischnic
Copy link
Contributor Author

mischnic commented Dec 9, 2020

I've just tested this in my project and it seems to work now

@ai ai merged commit 2326772 into postcss:main Dec 9, 2020
@mischnic mischnic deleted the fixup-fromJSON branch December 9, 2020 11:49
@ai
Copy link
Member

ai commented Dec 9, 2020

BTW, why do you need JSON AST rehydration?

@mischnic
Copy link
Contributor Author

mischnic commented Dec 9, 2020

For Parcel, I want to send ASTs across worker threads/cache them to disk.
(In this case, for CSS modules tree shaking: parcel-bundler/parcel#5363. Previously, we just stringified the AST before that).

Thanks you so much for the fast process!

@ai
Copy link
Member

ai commented Dec 9, 2020

Released in 8.2.1

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

2 participants