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

Serialize function AST instead of source #6

Closed
josephfrazier opened this issue Aug 3, 2017 · 7 comments · Fixed by #7
Closed

Serialize function AST instead of source #6

josephfrazier opened this issue Aug 3, 2017 · 7 comments · Fixed by #7

Comments

@josephfrazier
Copy link
Contributor

josephfrazier commented Aug 3, 2017

Hey @borisdiakur, thanks for the module! I was wondering if you'd be willing to accept a patch that would allow the comments/formatting of a function to change, without invalidating the cache. The idea is that instead of hashing the source code of the function, we would use a parser like babylon to build an AST (abstract syntax tree) of the function that represents its semantic behavior while disregarding comments/formatting/etc. The AST would be serialized into JSON and then included in the hash.

@borisdiakur
Copy link
Owner

Hi @josephfrazier. Sure. Creating the syntax tree itself may be expansive and changing the current behaviour would require a major version bump. So you’d probably make it an option and call it 'ast' or what ever.

@josephfrazier
Copy link
Contributor Author

Great, I'll see what I can come up with. I agree that it should be an option.

Note to self: Ideally, this would eventually be resistant to variable renaming within the function, but that would require a more sophisticated analysis (using something like http://esprima.org/demo/rename.html).

josephfrazier added a commit to josephfrazier/memoize-fs that referenced this issue Aug 26, 2017
josephfrazier added a commit to josephfrazier/memoize-fs that referenced this issue Aug 26, 2017
@borisdiakur
Copy link
Owner

Thanks @josephfrazier!

@josephfrazier
Copy link
Contributor Author

Thanks for the quick merge/release! How would you feel about the astBody feature automatically looking for cached file paths generated without astBody set, and migrating them to their new location? This would make it easier to start using astBody without busting pre-existing caches.

@borisdiakur
Copy link
Owner

My gut tells me that migration tasks should be avoided in production code. They should rather be implemented in a deployment pipeline.

@josephfrazier
Copy link
Contributor Author

josephfrazier commented Aug 26, 2017

That makes sense. In order for such a migration to be possible, I think we should export getCacheFilePath(). What do you think?

@borisdiakur
Copy link
Owner

Yes, that sounds reasonable.

josephfrazier added a commit to josephfrazier/memoize-fs that referenced this issue Aug 26, 2017
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 a pull request may close this issue.

2 participants