-
Notifications
You must be signed in to change notification settings - Fork 65
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
Source map support #163
Merged
Merged
Source map support #163
Changes from all commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
769a432
source-map: Create SourceNodes for functions and initializers
Mingun fcae22a
source-map: Generate tables of constants as a SourceNode
Mingun de0ad72
source-map: Remove excess `.join("\n")`s
Mingun 4ad80de
source-map: Do not join arrays prematurely
Mingun 9bdf8eb
source-map: Remove yet another premature join
Mingun fa5da47
source-map: Remove useless function
Mingun d482541
source-map: Generate top-level as a SourceNode
Mingun 6eece33
source-map: Generate module wrappers as a SourceNode
Mingun 8b7d69d
source-map: Represent generated code in the AST as a SourceNode
Mingun b352e92
source-map: Don't Repeat Yourself - introduce new interface SourceOpt…
Mingun c08e4d8
source-map: Implement API for producing source maps
Mingun cadc935
source-map: Run CLI tests in the test/cli directory
Mingun ef9f014
source-map: Do not break process until generated code/source map will…
Mingun e47b5f4
source-map: Add new `-m/--source-map` parameter to the CLI
Mingun e5b1ebf
source-map: Introduce helper for generate a parse rule function name
Mingun 1ee49ab
source-map: Add mapping for the rule names to the names of the parse …
Mingun fe39cbf
source-map: Cover ast2SourceNode completely
Mingun 6f57b4a
source-map: Alternative API: new output mode "source-and-map" instead…
Mingun 4ee3472
source-map: Make source-map work in the browser
hildjj 0ac78fd
Update all dependencies
Mingun 53f3d12
Only run lint on latest node, rather than holding back eslint to an o…
hildjj File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This worked when I added a
//# sourceMappingURL=
comment to the end of the generated .js file, which I think should be done automatically.This pointed out that the filenames in the .map file might need to be tweaked; from the project root, try
./bin/peggy.js examples/fizzbuzz.peggy --source-map
. The map file includes"sources":["examples/fizzbuzz.peggy"]
which should probably instead be `"sources":["fizzbuzz.peggy"]". The filename should be relative to the .map file.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that automatic appending of this line will be desired behavior in each case, mostly because I don't known how people will use source-map feature. Source maps could be hosted not in the same directory where it was generated and only the user know the right place. Adding this line is a trivial task, so I leaved it out of scope of my PR. We can firstly gather the feedback and add this line later if users will complain about it. Or, if you have a clear vision, I can add it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the line should be auto-appended in the CLI only. We can add an option for inlining it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
I'll leave that for the next PRs. There a many options to forming such URL. For example, someone can want to form a data URL embedding generated source map in it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is going to take a few more PRs before we're ready to release, and I don't want to docs to be wrong for that long a time, so I'm going to create a "1.3" branch that we can merge this into, and start to merge other work into as well.
After this lands in the 1.3 branch, and we add in #199, we should have a better way to talk about how to test whether the maps are working as expected. Make sense @Mingun?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think this is a good plan