Skip to content

fix(emit): Omit keys from sourcemap object if sourcemaps are enabled #12

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

Merged
merged 1 commit into from
Apr 30, 2019
Merged

Conversation

greglockwood
Copy link
Contributor

What problem does this solve?

Currently, if the sourceMap option for css-loader is set to true, the emitted interface will contain keys from the sourcemap object, such as file, mappings, names, sourceRoot and sources, which it should not do.

Main changes

  1. Increased specificity of keyRegex so it only matches the strings in the final map, not the sourceMaps object's keys.
  2. Added unit test for new behavior. Had to modify the compiler test function to allow populating the sourceMap option for css-loader.

'utf-8'
);

expect(declaration).toMatchInlineSnapshot(`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used an inline snapshot here because I wanted to have full control over what it was matching against. It also allowed me to add the test first, verify it failed, and then add my fix and re-run the test to verify it passes now. I could not have done that as easily if I generated the snapshots with the broken code.

@greglockwood
Copy link
Contributor Author

Do I need to bump the package version as well as part of this PR, to make sure people get the fix?

@michaeltaranto
Copy link
Contributor

@greglockwood all good, we use semantic-release so CI will publish for us. By providing fix as the type of commit, a patch version would be published.

Copy link
Member

@markdalgleish markdalgleish left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the fix 👍

@markdalgleish markdalgleish merged commit e2b1502 into seek-oss:master Apr 30, 2019
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.

3 participants