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.

Sign up for GitHub

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

Jump to bottom

Make eslint aware of the custom import paths #2292

Merged
merged 3 commits into from
Apr 17, 2018
Merged

Make eslint aware of the custom import paths #2292

merged 3 commits into from
Apr 17, 2018

Conversation

terebentina
Copy link
Contributor

@terebentina terebentina commented Mar 20, 2018 •
edited
Loading

By default eslint doesn't know what src/Renderer means in the following import:
import { Renderer } from 'src/Renderer';
This changes makes it aware that src/Renderer means <project root>/src/Renderer.js

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

@jsnellbaker jsnellbaker requested a review from mike-chowla March 20, 2018 19:20
@mike-chowla
Copy link
Contributor

The Travis CI build is failing.

Why are custom import paths needed?

@terebentina
Copy link
Contributor Author

Haha, the fact that it is failing means this PR already proved its usefulness :)
The eslint check in Travis fails here:
https://github.com/prebid/Prebid.js/blob/master/modules/sonobiBidAdapter.js#L2-L3
Until now eslint didn't know that src/utils and ../src/utils are one and the same thing (in this case). Now it knows and it correctly identified this problem - you shouldn't import the same file multiple times.

To answer your question: line 2 in that adapter is an absolute import. By default nodejs resolves absolute imports relative to the node_modules folder. In this case, there's no node_modules/src/utils.js path so if this would be run directly with nodejs it would fail.
You are passing all files through webpack though and already told webpack how it should resolve these special cases: https://github.com/prebid/Prebid.js/blob/master/webpack.conf.js#L15-L20 : look for the file either in node_modules or in the root folder.
This PR simply makes eslint aware of the same rules.

@mkendall07
Copy link
Member

@terebentina

Thanks for the PR. Can you update the PR so it passes (fix the import in the sonobi adapter etc). Thanks

@mkendall07
Copy link
Member

bump

@terebentina
Copy link
Contributor Author

Sorry, yes. This weekend!

@terebentina
Make eslint aware of the custom import paths
be59d07 
By default eslint doesn't know what `src/Renderer` means in the following import:
`import { Renderer } from 'src/Renderer';`
This changes makes it aware that `src/Renderer` means `<project root>/src/Renderer.js`
@terebentina
fix: fix import paths in sonobiBidAdapter.js
7f0cbdb
@terebentina
fix: imports in bidderFactory.js
9d56589
@terebentina
Copy link
Contributor Author

all good!

mkendall07
mkendall07 approved these changes Apr 17, 2018
View reviewed changes
Copy link
Member

@mkendall07 mkendall07 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

@mkendall07 mkendall07 added LGTM needs 2nd review Core module updates require two approvals from the core team and removed needs update labels Apr 17, 2018
@jaiminpanchal27 jaiminpanchal27 merged commit a67da48 into prebid:master Apr 17, 2018
ArmandChoy pushed a commit to RockYou-Ads/Prebid.js that referenced this pull request Apr 18, 2018
Merge branch 'master' of https://github.com/prebid/Prebid.js
e9f72f2 
* 'master' of https://github.com/prebid/Prebid.js: (211 commits)
  Increment Pre Version
  Prebid 1.8.0 Release
  Make eslint aware of the custom import paths (prebid#2292)
  send travis-ci notifications to slack (prebid#2404)
  send appnexus usePaymentRule info to prebid-server ortb request (prebid#2351)
  convert AN bid params to underscore formatting for pbs (prebid#2385)
  EbdrAdapter add usersync  (prebid#2407)
  Add outstream renderer to Beachfront adapter (prebid#2403)
  Add analytics adapter by Sigmoid (prebid#2316)
  deprecate loadScript and add loadExternalScript (prebid#2391)
  Removed the ability for to override any standard query parameters (prebid#2402)
  Unit test failures (prebid#2405)
  Add Unruly Bid Adapter (prebid#2326)
  Added VIS.X Bidder Adapter (prebid#2359)
  Smart: Add prebid version in the data payload (prebid#2394)
  add support for video bids to use an impression tracking URL  (prebid#2365)
  Create rtbdemandAdkBidAdapter_spec.js (prebid#2352)
  Widespace adapter (prebid#2283)
  Add: Vuble Analytics Adapter (prebid#2331)
  fixes prebid#2353 - not appending hb_uuid and hb_cache_id (prebid#2363)
  ...

# Conflicts:
#	test/spec/modules/rockyouBidAdapter_spec.js
dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
@terebentina @dluxemburg
Make eslint aware of the custom import paths (prebid#2292)
32dff16 
* Make eslint aware of the custom import paths

By default eslint doesn't know what `src/Renderer` means in the following import:
`import { Renderer } from 'src/Renderer';`
This changes makes it aware that `src/Renderer` means `<project root>/src/Renderer.js`

* fix: fix import paths in sonobiBidAdapter.js

* fix: imports in bidderFactory.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaiminpanchal27 jaiminpanchal27 jaiminpanchal27 approved these changes

@mkendall07 mkendall07 mkendall07 approved these changes

@mike-chowla mike-chowla Awaiting requested review from mike-chowla

Assignees

@mike-chowla mike-chowla

Labels
LGTM needs 2nd review Core module updates require two approvals from the core team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@terebentina @mike-chowla @mkendall07 @jaiminpanchal27 @jsnellbaker