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

Add source-with-inline-map #282

Merged
merged 5 commits into from
Jun 10, 2022
Merged

Conversation

hildjj
Copy link
Contributor

@hildjj hildjj commented Jun 9, 2022

Fixes #280.

Copy link
Member

@Mingun Mingun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally approve, but need to review doc comments about grammarSource.

Comment on lines 421 to 422
<code>"source-with-inline-map"</code>. The path should be relative to
the location where the generated parser code will be stored.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last sentence seems not relevant for source-with-inline-map

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is, actually. If you generate with grammar source "src/foo.peggy", and write the output file to "lib/foo.peggy", then the grammar source needs to be "../src/foo.peggy". On the CLI, you can use fully-qualified paths, because i use node's path.relative to fix them up. Here, I don't want to pull in path or a polyfill, so I just change the requirement so that the grammarSource has to be relative.

Copy link
Member

@Mingun Mingun Jun 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never-mind, my mistake. I mistakenly thought that it is the path to the file into which the source map is embed, that is why I was doubt is it required to have a concrete value at all.

However, perhaps add your example to the doc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An example is a great idea. Adding.

lib/compiler/utils.js Show resolved Hide resolved
lib/peg.d.ts Outdated
Comment on lines 1218 to 1190
* Note, that `SourceNode.source`s of the generated source map will depend
* on the `options.grammarSource` value. Therefore, value `options.grammarSource`
* will propagate to the `sources` array of the source map. That array MUST
* contain a path relative to the source map location, as no further path
* processing will be performed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That part seems incorrect. What location the inline source map have?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be worded better. The important point is that the grammarSource is a string with a path relative to where the .js file will eventually be written.


it("generates inline sourceMappingURL", () => {
const ast = parser.parse("foo='1'");
// Ensure there is an expect even if we don't run the good tests below.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Ensure there is an expect even if we don't run the good tests below.
// Ensure there is an object even if we don't run the good tests below.

Misprint, I guess

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eh, i'll just remove the comment. what i'm aiming for is that if you're running in an old browser, you don't see an error, and the it contains at least one expect.

@hildjj
Copy link
Contributor Author

hildjj commented Jun 9, 2022

Fixed two doc issues, added changelog, and fixed #283 while I was in there.

@hildjj hildjj force-pushed the source-with-inline-map branch from 1a3e05b to b9f5e85 Compare June 10, 2022 16:50
@hildjj
Copy link
Contributor Author

hildjj commented Jun 10, 2022

I'll rebase to squash the extra commits before landing. Left them separate so you could see the changes for now.

@hildjj hildjj mentioned this pull request Jun 10, 2022
@hildjj hildjj force-pushed the source-with-inline-map branch from b9f5e85 to e879502 Compare June 10, 2022 18:07
@hildjj hildjj merged commit 46878cd into peggyjs:main Jun 10, 2022
@hildjj hildjj deleted the source-with-inline-map branch June 10, 2022 18:10
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.

New output type for inline sourcemap
2 participants