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

NPM usage doc: Add resolve.extensions #4856

Closed
wants to merge 1 commit into from

Conversation

ckybonist
Copy link

Type of change

  • Bugfix
  • Feature
  • New bidder adapter
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Does this change affect user-facing APIs or examples documented on http://prebid.org?
  • Other

Description of change

Add resolve.extensions to the section

  • test parameters for validating bids
{
  bidder: '<bidder name>',
  params: {
    // ...
  }
}

Be sure to test the integration with your adserver using the Hello World sample page.

  • contact email of the adapter’s maintainer
  • official adapter submission

For any changes that affect user-facing APIs or example code documented on http://prebid.org, please provide:

Other information

@snapwich
Copy link
Collaborator

Does webpack not resolve .js and .json files by default? I'm pretty sure it does.

@ckybonist
Copy link
Author

Does webpack not resolve .js and .json files by default? I'm pretty sure it does.

Absolutely it does.

However, IMHO, users who maintain typescript-based projects (like me) would definitely specify the resolve.extensions for .ts; Without the clear documentation, we have to dig into node_modules/prebid.js to find out which file causing the error, moreover, the webpack error logs
can't even tell us the actual reason why I get tripped...

Module not found: Error: Can't resolve '../src/constants' 
in '/Users/FatCat/my-project/node_modules/prebid.js/modules'

// Multiple errors...

NOTE: no extension at ../src/constants

@snapwich
Copy link
Collaborator

So you're saying you changed resolve.extensions to .ts and it caused Prebid.js modules to not be found? Seems like expected behavior (hindsight is 20/20 I know).

I'd rather not specify default configuration, or configuration at all, in the webpack config that doesn't relate specifically to Prebid.js as that would include A LOT of stuff besides just resolving extensions depending on what people are doing with webpack.

If being explicit in our code and manually specifying the .js extensions in all Prebid.js imports works around this gotcha, that seems like a better solution. Then we can add this ESLint rule that ensures that all code going forward uses extensions.

@snapwich snapwich mentioned this pull request Feb 19, 2020
1 task
@snapwich
Copy link
Collaborator

this issue should be resolved by #4876 being merged. let me know if this doesn't work for you.

@snapwich snapwich closed this Feb 19, 2020
@ckybonist
Copy link
Author

ckybonist commented Feb 20, 2020

Actually, I changed resolve.extensions to ['.js', '.ts'] at first and that's why I get the errors (i.e. missing .json).

Thanks for specifying all module extension explicitly! That's truly a BIG PR.

Waiting for next stable release!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants