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

custom namespacing - use PrebidGlobal in the source code, replace it with pbjs as a webpack step #293

Closed
wants to merge 3 commits into from
Closed

Conversation

baragona
Copy link
Contributor

The name "PrebidGlobal" was chosen as it's less likely to cause conflicts. A webpack step will convert every instance of that string with "pbjs" -- configurable in package.json .

Why do this with webpack instead of a gulp task? Because when running the tests with karma, only webpack is applied, no other operations that gulp is configured to apply.

Currently the tests only pass if the global name is configured to be 'pbjs'. That's because I did not rename all usages of pbjs to PrebidGlobal in the tests (yet). See this in the karma conf:

    preprocessors: {
      'test/**/*_spec.js': ['webpack'],
      'src/**/*.js': ['webpack', 'coverage']
    },

Is that going to apply webpack to all the test files that may need substitution?

Hope this helps.

…bjs (or anything else) with a webpack plugin. the name is configurable in package.json
@baragona
Copy link
Contributor Author

baragona commented Apr 1, 2016

By the way I won't be able to collect any stats on whether this breaks anything until tomorrow.

@baragona
Copy link
Contributor Author

baragona commented Apr 4, 2016

update: I'm running this in production with a bunch of adapters and don't see any issues.

@mkendall07
Copy link
Member

@ojotoxy
Thanks for the cleaned up PR. We are going to change the global name to something like $$PREBID_GLOBAL$$ which still is a valid identifier and also makes it more clear it will be string replaced.

@protonate
Copy link
Collaborator

I tried a quick replace of PrebidGlobal with $$PREBID_GLOBAL$$ and we will also need to replace the global var in test files and example files in ./integrationExamples/gpt/, so the StringReplacePlugin loader in webpack.conf needs to include "integrationExamples". Then if you would squash commits, we'll have another review and get this merged.

@nedstankus
Copy link
Contributor

What's the plan for this feature? I'd love to see it in the next release and am able to help with the code if there's a need.

@protonate
Copy link
Collaborator

Thanks @nedstankus PRs are welcome. We stalled on this work and are at the point where the changes should be applied fresh to current master. The global var would need to be changed in all src and test files.

@mkendall07
Copy link
Member

@ojotoxy
Closing out this PR since it's stale. Would still accept a new PR for this feature using $$PREBID_GLOBAL$$ as the param for pbjs replacement. Thanks

@mkendall07 mkendall07 closed this Jun 13, 2016
@nedstankus
Copy link
Contributor

nedstankus commented Jun 14, 2016

I've ported these changes to v0.9 and submitted a new PR:
#409

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