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

Build Related : rename constants.json to constants.js #11292

Merged
merged 30 commits into from
Apr 15, 2024

Conversation

muuki88
Copy link
Collaborator

@muuki88 muuki88 commented Apr 4, 2024

Type of change

  • Build related changes

Description of change

Rename constants.json to constants.js

TODOs left

  • export separate consts
  • if something imported only by one module, it should be moved into that module
  • if something's imported only by modules, it should be moved into libraries

Other information

gulpfile.js Outdated Show resolved Hide resolved
@muuki88
Copy link
Collaborator Author

muuki88 commented Apr 4, 2024

'Uncaught exception:', TypeError: Cannot read properties of undefined (reading 'AUCTION_INIT')

nooo

@muuki88
Copy link
Collaborator Author

muuki88 commented Apr 4, 2024

Running a single test works

gulp test --file "test/spec/modules/yieldoneBidAdapter_spec.js" --nolint

running all tests, it fails. yyyyyyyyyyyyyy .... @dgirardi help plz

src/constants.js Outdated
@@ -0,0 +1,196 @@
export default {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is the same as JSON for the minifier. It wouldn't be able to remove dead definitions; I'm curious and I'd bet it also wouldn't fix the problem you were having.

If it doesn't, what we want is

export const EVENTS = {...}; 
export const TARGETING_KEYS = {...};
...

or even better would be

export const EVENT_AUCTION_INIT = 'auctionInit';
...
export const TARGETING_KEY_BIDDER = 'hb_bidder';
...

but of course the latter is a lot more work. I'm sorry I didn't make this clearer earlier.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No worries. The first one I'll definitely do.

The issue currently is that the minifier removes too much 😅

@muuki88
Copy link
Collaborator Author

muuki88 commented Apr 5, 2024

I found the issue why constants is not defined. I messed up some imports

@muuki88 muuki88 requested a review from dgirardi April 5, 2024 11:26
src/constants.js Outdated Show resolved Hide resolved
src/constants.js Outdated Show resolved Hide resolved
src/constants.js Outdated Show resolved Hide resolved
@GeneGenie
Copy link
Contributor

GeneGenie commented Apr 11, 2024

This is such a win for my current research, i think i could have branched from here and go fully on node.
#11327

@muuki88
Copy link
Collaborator Author

muuki88 commented Apr 12, 2024

Awesome 🥳

Is there anything I can do to push this? Otherwise updating and resolving merge conflicts becomes cumbersome.

And we hit yet another issue with a bidder that doesn't work since the PUC refactoren, which is probably caused by some webpack minification, trimming away actually required constants 🫠

@ChrisHuie ChrisHuie changed the title Fix #10829 constants json Build Related : rename constants.json to constants.js Apr 15, 2024
@ChrisHuie ChrisHuie merged commit ed2b823 into prebid:master Apr 15, 2024
4 checks passed
@muuki88 muuki88 deleted the fix-10829-constants-json branch April 15, 2024 20:27
@muuki88
Copy link
Collaborator Author

muuki88 commented Apr 15, 2024

Thanks @ChrisHuie
thanks

DecayConstant pushed a commit to mediavine/Prebid.js that referenced this pull request Jul 18, 2024
* Fix lintWarnings gulp parameter

* Fix prebid#10829 Rename constants.json to constant.js

* Remove unnecessary quotes

* Fix import in genericAnalyticsAdapter

* remove unnecessary quotes

* Change require to imports

* Revert "Fix lintWarnings gulp parameter"

This reverts commit e210f4e.

* Add nolint and no-lint-warnings docs to README

* WIP refactoring constants into separated exported files

* Refactor the rest of imports

* Fix invalid import

* Add missing MESSAGSES import

* Add missing S2S import

* Fix broken constants.js import

* Use proper events import in magnite adapter

* Add import to pubmatic spec

* Found another CONSTANTS import

* Fix shadowed var

* fix zeta global test

* Add missing BID_STATUS import

* Fix  zeta global ssp analytics for real

* fix lint error

* Remove duplicated TARGETING_KEYS definition

* Move FLOOR_SKIPPED_REASON into priceFloors.js

* Remove unused CB constants

* Move FLOOR_SKIPPED_REASON into pubmaticAnalyticsAdapter

* Use proper import

* Remove unused var and fix linting error
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.

4 participants