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

require extensions in imports #4876

Merged
merged 4 commits into from
Feb 19, 2020
Merged

require extensions in imports #4876

merged 4 commits into from
Feb 19, 2020

Conversation

snapwich
Copy link
Collaborator

Type of change

  • Refactoring (no functional changes, no api changes)

Description of change

Explicitly require extensions on imports and validate with eslint plugin.

Other information

This should fix any issues with dependent projects including Prebid.js that don't have the default extensions as part of the module resolver as described in #4856

@@ -638,7 +638,7 @@ export const spec = {
bidObject.adserverTargeting = extPrebidTargeting;
}

// try to get cache values from 'response.ext.prebid.cache'
// try to get cache values from 'response.ext.prebid.cache.js'
Copy link
Contributor

Choose a reason for hiding this comment

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

is this change necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nope, that was an accidental catch by the regex replace

@@ -1,8 +1,7 @@
import 'mocha/mocha';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Guessing this is an unused import

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I sure hope so, I can't imagine what it would be doing where it'd need to be included only for its side-effects... either way I removed it and the tests still passed so... good?

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@pm-harshad-mane
Copy link
Contributor

Looks good to me, but please check the failing CI Job.

@robertrmartinez robertrmartinez self-requested a review February 19, 2020 22:07
Copy link
Collaborator

@robertrmartinez robertrmartinez left a comment

Choose a reason for hiding this comment

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

Couldn't find any glaring issues LGTM!

Tests working is a good sign and we can do some other verifications no other errors thrown in the console when loading the entire bundle.

Thanks Rich!

@snapwich snapwich merged commit 1b0dfc2 into master Feb 19, 2020
@snapwich snapwich deleted the extensions branch February 19, 2020 23:50
@jaiminpanchal27 jaiminpanchal27 mentioned this pull request Feb 25, 2020
2 tasks
@snapwich snapwich mentioned this pull request Feb 28, 2020
1 task
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