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

fix(client): add default fallback for client #2015

Merged
merged 6 commits into from
Jun 17, 2019

Conversation

knagaitsev
Copy link
Collaborator

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

Not yet, probably should resolve asap

Motivation / Use-Case

Fixes #2006 by providing a default fallback in case __webpack_dev_server_client__ is undefined.

I moved the default client sources to client/default so that the file path structure is the same, allowing socket.js to require('../clients/SockJSClient') in both client and client-src

Breaking Changes

None

Additional Info

We still need to figure out how to insert __webpack_dev_server_client__ variable into live mode.

@codecov
Copy link

codecov bot commented Jun 11, 2019

Codecov Report

Merging #2015 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2015   +/-   ##
=======================================
  Coverage   92.77%   92.77%           
=======================================
  Files          29       29           
  Lines        1149     1149           
  Branches      327      327           
=======================================
  Hits         1066     1066           
  Misses         79       79           
  Partials        4        4

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 303f4e9...d15d7d9. Read the comment docs.

// this SockJSClient is here as a default fallback, in case inline mode
// is off or the client is not injected. This will be switched to
// WebsocketClient when it becomes the default
const SockJSClient = require('../clients/SockJSClient');
Copy link
Member

Choose a reason for hiding this comment

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

Move to L19

hiroppy
hiroppy previously approved these changes Jun 11, 2019
@hiroppy
Copy link
Member

hiroppy commented Jun 11, 2019

We should release as a patch version asap. Thanks.

@knagaitsev
Copy link
Collaborator Author

@evilebottnawi Looks good?

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Good job, some notes and question

// eslint-disable-next-line global-require
const SockJSClient = require('../clients/SockJSClient');
Client = SockJSClient;
}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe more simple? Like:

const Client = typeof __webpack_dev_server_client__ !== 'undefined' ? __webpack_dev_server_client__ : require('../clients/SockJSClient');

@@ -20,7 +20,7 @@ function addEntries(config, options, server) {
const sockPath = options.sockPath ? `&sockPath=${options.sockPath}` : '';
const sockPort = options.sockPort ? `&sockPort=${options.sockPort}` : '';
const clientEntry = `${require.resolve(
'../../client/'
'../../client/default/'
Copy link
Member

Choose a reason for hiding this comment

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

Breaking change, some packages can use this directly, so changing path can break these apps

package.json Outdated
@@ -25,7 +25,7 @@
"test": "npm run test:coverage",
"pretest": "npm run lint",
"prepare": "rimraf ./ssl/*.pem && npm run build:client",
"build:client:default": "babel client-src/default --out-dir client --ignore \"./client-src/default/*.config.js\"",
"build:client:default": "babel client-src/default --out-dir client/default --ignore \"./client-src/default/*.config.js\"",
Copy link
Member

Choose a reason for hiding this comment

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

Breaking change, as i written above

@knagaitsev
Copy link
Collaborator Author

@evilebottnawi any ideas how to make require('../clients/SockJSClient') resolve to the right place in both client and client-src?

@alexander-akait
Copy link
Member

@Loonride hm, we already keep clients in difference directory, can't see problem

@knagaitsev
Copy link
Collaborator Author

knagaitsev commented Jun 13, 2019

@Loonride hm, we already keep clients in difference directory, can't see problem

The problem is the directory organization of client and client-src are not the same. Therefore, when socket.js in client-src uses require('../clients/SockJSClient') it will work

When socket.js in client uses require('../clients/SockJSClient'), it does not work, the correct path would be ./clients/SockJSClient.

And we need both to work, because it needs to be functional in the babel transpiled client, but it also needs to function in client-src so that webpack can compile it to a bundle.

@alexander-akait
Copy link
Member

Good catch, i think we can change location and create index.js with content like:

require('./default/index')

What do you think?

@knagaitsev
Copy link
Collaborator Author

Good catch, i think we can change location and create index.js with content like:

require('./default/index')

What do you think?

That is a good idea, let me push an alternative solution that I just made though which may be simpler and we can compare.

@knagaitsev knagaitsev dismissed stale reviews from wizardofhogwarts and hiroppy via 8c90abb June 13, 2019 16:08
@knagaitsev
Copy link
Collaborator Author

@evilebottnawi my idea is to use webpack.NormalModuleReplacementPlugin to direct the paths to the right place. The only problem I have with this is that it makes the files in client-src not work unless accompanied by the proper webpack config. But users should really not be using stuff in client-src anyway, right?

@knagaitsev
Copy link
Collaborator Author

@evilebottnawi thoughts?

@knagaitsev
Copy link
Collaborator Author

@evilebottnawi @hiroppy Could you take a look so we can resolve this issue?

client-src/default/webpack.config.js Show resolved Hide resolved
@@ -33,5 +34,11 @@ module.exports = {
to: path.resolve(__dirname, '../../client/live.html'),
},
]),
new webpack.NormalModuleReplacementPlugin(/\/clients\//, (resource) => {
Copy link
Member

Choose a reason for hiding this comment

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

memo: We will delete this plugin at the next major version because we can change the directory structure.

@hiroppy
Copy link
Member

hiroppy commented Jun 17, 2019

I'll release today. thanks!

@hiroppy hiroppy merged commit d26b444 into webpack:master Jun 17, 2019
@alexander-akait
Copy link
Member

alexander-akait commented Jun 18, 2019

Don't do this in future without my approve, here bug, other module(s)/packages can have clients name and we break these apps
/cc @hiroppy @Loonride

@alexander-akait
Copy link
Member

We should do regexp more strictly now all clients directories in requests rewrites in all modules, i.e. if package use clients directory we rewrite their import/require too, it is very very very big problem, I have many issues about this in DM gutter and other channels and I don't access to fix it right now, please guys take care about it asap, it is incredibly high priority

@hiroppy
Copy link
Member

hiroppy commented Jun 18, 2019

@evilebottnawi Would you want to revert? We should add the state actually used by userland to the test.

@knagaitsev
Copy link
Collaborator Author

@evilebottnawi Sorry about that, I did not think it would impact other modules.

@hiroppy I say revert, then we follow the other method that @evilebottnawi suggested

@knagaitsev knagaitsev added gsoc Google Summer of Code scope: ws(s) labels Aug 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gsoc Google Summer of Code scope: ws(s)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uncaught ReferenceError: __webpack_dev_server_client__ is not defined
4 participants