Skip to content
This repository has been archived by the owner on Sep 9, 2021. It is now read-only.

feat: add publicPath support (options.publicPath) #31

Merged

Conversation

DullReferenceException
Copy link
Contributor

@DullReferenceException DullReferenceException commented Sep 30, 2016

Our web app hosts static assets on a CDN, making the loading of web worker scripts difficult due to cross-origin policies. The inline option works for most browsers, but we started experiencing some difficulties with Safari due to its handling of Unicode in blobs being inconsistent with other browsers. This proxy option lets the worker load from a same-origin URL; in our case, we have some middleware in which we proxy the request to the webpack dev server (for local development) or serve up a local file.

Noteable Changes

Added the ability to use a proxy URL when loading workers as a way to work around cross-origin policy issues.

ℹ️ edited by @michael-ciniawsky (Formatting)

Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

Needs Rebase && Tests

.gitignore Outdated
node_modules
.idea
Copy link
Member

Choose a reason for hiding this comment

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

Please revert this

@michael-ciniawsky
Copy link
Member

Can you rebase please ? So I can finally review this ? :)

@jsf-clabot
Copy link

jsf-clabot commented Oct 12, 2017

CLA assistant check
All committers have signed the CLA.

@DullReferenceException
Copy link
Contributor Author

@michael-ciniawsky: rebased and added a unit test now that unit tests are supported.

Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

Maybe better to name the option publicPath instead ?

cc @TrySound

index.js Outdated
@@ -9,7 +9,8 @@ const SingleEntryPlugin = require('webpack/lib/SingleEntryPlugin');
const schema = require('./options.json');

const getWorker = (file, content, options) => {
const workerPublicPath = `__webpack_public_path__ + ${JSON.stringify(file)}`;
const basePath = options.proxy ? JSON.stringify(options.proxy) : '__webpack_public_path__';
Copy link
Member

@michael-ciniawsky michael-ciniawsky Oct 12, 2017

Choose a reason for hiding this comment

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

basePath => publicPath or inline it into the template literal

@michael-ciniawsky michael-ciniawsky changed the title feat: add proxy support (options.proxy) feat: add custom publicPath support (options.publicPath) Oct 12, 2017
Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Oct 12, 2017

@DullReferenceException Could you just add /* eslint-disable comma-dangle, arrow-body-style */ at the top in test/index.js please so CI passes?

@michael-ciniawsky
Copy link
Member

or fix them all good 😛

@michael-ciniawsky michael-ciniawsky changed the title feat: add custom publicPath support (options.publicPath) feat: add publicPath support (options.publicPath) Oct 14, 2017
@michael-ciniawsky
Copy link
Member

@DullReferenceException Please one last rebase sry 😛

@TrySound @d3viant0ne @evilebottnawi Any objections against this ? Please review when time 🙃

README.md Outdated
@@ -151,3 +170,8 @@ const worker: Worker = new MyWorker();

[test]: http://img.shields.io/travis/webpack-contrib/worker-loader.svg
[test-url]: https://travis-ci.org/webpack-contrib/worker-loader


## License
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be here but should go away on the last rebase

README.md Outdated
@@ -91,6 +91,25 @@ self.postMessage({foo: 'foo'})
self.addEventListener('message', (event) => { console.log(event); });
```


## Cross-origin policy workarounds

Copy link
Member

Choose a reason for hiding this comment

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

Can we describe the feature to be less use case specific?

Feel free to add the worker cross-origin policy as an example underneath.

  • Add publicPath to table that describes the api
  • Add ## Public Path docs below the existing sections describing the api options
  • Add generic example
  • Add note about workaround for cross-origin specific usecase.

Copy link
Member

Choose a reason for hiding this comment

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

@michael-ciniawsky Other than the documentation, it looks fine pending a thumbs up from @TrySound

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 I've rebased & updated the documents.

Added the ability to use a different public path when loading workers as a way to work around cross-origin policy issues.
Copy link
Contributor

@TrySound TrySound left a comment

Choose a reason for hiding this comment

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

Looks good

Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

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

Successfully merging this pull request may close these issues.

7 participants