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

Fix error when parsing keys with hyphen in some JSON files #7316

Merged
merged 4 commits into from
Nov 28, 2023

Conversation

DontBreakAlex
Copy link
Contributor

@DontBreakAlex DontBreakAlex commented Nov 25, 2023

What does this PR do?

Fixes #7261 introduced by #4783.
The solution is to quote export aliases when required so that

var key_with_hyphen = !0

export {
    key_with_hyphen as key-with-hyphen
};

export default { "key-with-hyphen": key_with_hyphen };

becomes

var key_with_hyphen = !0

export {
    key_with_hyphen as "key-with-hyphen"
};

export default { "key-with-hyphen": key_with_hyphen };

Relevant: tc39/ecma262#2154 and https://bugs.webkit.org/show_bug.cgi?id=217576

How did you verify your code works?

I wrote automated tests

  • I checked the lifetime of memory allocated to verify it's (1) freed and (2) only freed when it should be
    I did not add allocations
  • I included a test for the new code, or an existing test covers it
  • I wrote TypeScript/JavaScript tests and they pass locally (bun-debug test 07261.test.ts)

src/bundler.zig Outdated
@@ -1435,12 +1435,18 @@ pub const Bundler = struct {
}, prop.key.?.loc),
.value = prop.value.?,
};
var maybe_quoted = name;
if (strings.containsChar(name, '-')) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there already a function that checks if a string is a valid variable name or do I need to write one ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, MutableString.ensureValidIdentifier. It will check if it's a valid identifier, and create a new one with the non-valid characters replaced by _ if it is not valid

src/bundler.zig Outdated
@@ -1435,12 +1435,18 @@ pub const Bundler = struct {
}, prop.key.?.loc),
.value = prop.value.?,
};
var maybe_quoted = name;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
var maybe_quoted = name;
var maybe_quoted = MutableString.ensureValidIdentifier(name, allocator) catch return null;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already use this function to create the name of temporary variable, and it replaces the hyphens with underscores. With your suggestion the code becomes this:

var key_with_hyphen = !0;

export {
  key_with_hyphen
};
export default { "key-with-hyphen": key_with_hyphen };

@DontBreakAlex DontBreakAlex force-pushed the fix-json-with-hypens branch 2 times, most recently from a711c7e to b939c8f Compare November 26, 2023 11:44
@DontBreakAlex
Copy link
Contributor Author

DontBreakAlex commented Nov 26, 2023

So I figured It was not the bundler's job to handle quoting stuff. I instead modified to printer to quote aliases if they are not a valid identifier. This removes the need for allocation.

I think this PR is ready to be merged.

@DontBreakAlex DontBreakAlex marked this pull request as ready for review November 26, 2023 12:33
@paperclover paperclover merged commit 3488192 into oven-sh:main Nov 28, 2023
18 of 29 checks passed
@paperclover
Copy link
Member

thank you

ryoppippi pushed a commit to ryoppippi/bun that referenced this pull request Feb 1, 2024
…es (oven-sh#7316)

* Quote export aliases with hyphens when converting jsons to modules

* Add test

* Handle quotes in printer and not in bundler

* [autofix.ci] apply automated fixes

---------

Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
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.

Error when parsing keys with hyphen in some JSON files
3 participants