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

[core-auth] Fix 'browser' path configuration in core-auth #4460

Merged
merged 1 commit into from
Jul 26, 2019

Conversation

daviwil
Copy link
Contributor

@daviwil daviwil commented Jul 26, 2019

This corrects the browser configuration in core-auth's package.json file to ensure that it points to the correct browser output file. Presumably this would only affect consumers who are trying to directly reference this library in a Rollup build.

It actually looks like sdk/template/template may have this configured incorrectly, that's where I copied it from. @bterlson: do you know if this file remapping in browser is needed for consumers of our libraries?

@daviwil daviwil requested review from bterlson and mikeharder July 26, 2019 18:56
@mikeharder
Copy link
Member

mikeharder commented Jul 26, 2019

I agree that template is configured incorrectly, but I'm surprised the template browser-based tests didn't fail because of this. The issue might be masked by the way we build the bundle for browser testing. I've created an issue for myself to investigate this, add tests, and fix any broken packages:

#4461

@daviwil
Copy link
Contributor Author

daviwil commented Jul 26, 2019

Thanks Mike! I confirmed with @bterlson offline that we should have these paths set correctly even if it seems Rollup doesn't resolve them in our current builds. He suggests that we could add a linting check for it too, possibly as a result of @arpanlaha's work.

For now I'll go ahead and merge this and then I can help you with the testing story for #4461.

@daviwil daviwil merged commit 9e05349 into Azure:master Jul 26, 2019
@daviwil daviwil deleted the fix-core-auth-browser branch July 26, 2019 19:43
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