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: throw error for invalid $refs missing the / #102

Merged
merged 9 commits into from
Oct 24, 2023

Conversation

darrenyong
Copy link

🚥 Resolves RM-7648

🧰 Changes

This fix seems a little... hard-coded? so I'm open to suggestions! It's literally just checking for the #components string anytime we try to dereference the file.

Additionally, I'm using some optional chaining but ESLint doesn't like it. Is it okay to update the parserOptions.ecmaVersion to 2020?

🧬 QA & Testing

I don't have a great way to test this but I've written a test for that specific case and all the other tests are passing.

@darrenyong darrenyong changed the title Fix/throw error for invalid refs fix: throw error for invalid $refs missing the / Sep 26, 2023
@kanadgupta kanadgupta self-requested a review September 26, 2023 14:36
lib/dereference.js Outdated Show resolved Hide resolved
@darrenyong darrenyong marked this pull request as ready for review October 19, 2023 21:48
lib/dereference.js Outdated Show resolved Hide resolved
Copy link
Member

@erunion erunion left a comment

Choose a reason for hiding this comment

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

looks like you've got a linting issue but otherwise this looks good!

@darrenyong
Copy link
Author

@erunion bumped the ecmaVersion to 2020 to fix the chaining lint issue. didn't look like there were other linting issues that came with the version bump so hopefully it's okay!

@erunion erunion merged commit a44c276 into main Oct 24, 2023
12 of 20 checks passed
@erunion erunion deleted the fix/throw-error-for-invalid-refs branch October 24, 2023 21:52
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.

2 participants