Module browser bundle configuration using webpack #48

schrepfler · 2016-11-15T17:52:56Z

Only thing I can see is that the bundle is being created but I don't have the skills to really test this (sorry, Java/Scala guy here).
This feature should enable simple CDN-like referencing via https://unpkg.com/ while keeping the publishing to npm as a basis. Related to #47

TODO:

Make sure all of the significant new logic is covered by tests
Rebase your changes on master so that they can be merged easily
Make sure all tests and linter rules pass
Update CHANGELOG.md with your change

apollo-cla · 2016-11-15T17:52:59Z

@schrepfler: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

schrepfler · 2016-11-15T23:27:49Z

To trigger the build one just launches webpack after which the browser bundle is created. This command can probably be added in package.json, I guess it depends on your build pipeline. Not sure if I need to add something specific so that npm publish publishes the artifact to npm as well?

schrepfler · 2016-11-16T18:40:17Z

At the moment I'm not sure if the generated bundle is ideal for consumption from </script> tags. Would be nice if a webpack expert suggests what's missing and also confirm will npm publish publish the browser bundle automatically or not?

schrepfler · 2016-11-18T11:58:55Z

I think at the moment the bundle is not properly created, don't merge just yet (and if anyone has suggestions feel free to comment on the PR.)

…et as umd

…s-transport-ws

schrepfler · 2016-11-20T04:07:52Z

I think with this final commit, the generated bundle is usable in a browser via a script tag

var client = new subscriptions_transport_ws.Client('ws://localhost:8080/graphql', {timeout: 4000})

and

npm pack

generates a tgz which contains it so I suppose this is more/less what we want?

schrepfler · 2016-11-20T17:17:47Z

The only thing which actually comes to mind, if subscriptions_transport_ws imports things from graphql-subscriptions and they finish in the bundle, it might be possible to say that these need to be provided as linked scripts from the outside (declare them external in webpack) but at this point I think it's better for you guys to evaluate.

schrepfler · 2016-12-03T22:31:26Z

Any feedback on this would be nice?

helfer · 2016-12-13T10:13:12Z

@schrepfler sorry, this fell off of my radar for a while. I think we should definitely implement it, but it looks like you're compiling things twice now (once with TS once with webpack). Couldn't webpack just use the output from TS and bundle for the browser after that?

@rricard I think you know this library (and also js in general 😄 ) pretty well. Could you take a quick look at this PR and help @schrepfler where necessary? That would be awesome!

rricard · 2016-12-13T10:24:20Z

@helfer thanks for directing me to this issue.

@schrepfler I'm going to do a proper review of your PR.

rricard

What you did works 🎉 (by eyeballing it, we'll need to actually publish it to be sure it really works... Like you I'm concerned about the peer dependency on graphql-subscriptions) so it's pretty cool, although, I have to nitpick on some stuff! Fix that and I'll be able to merge you!

rricard · 2016-12-13T10:25:51Z

package.json

@@ -24,7 +26,8 @@
    "testonly": "mocha --reporter spec --full-trace ./dist/test/tests.js",
    "coverage": "node ./node_modules/istanbul/lib/cli.js cover _mocha -- --full-trace ./dist/test/tests.js",
    "postcoverage": "remap-istanbul --input coverage/coverage.raw.json --type lcovonly --output coverage/lcov.info",
-    "prepublish": "npm run compile"
+    "start": "webpack --config webpack.config.js",


Don't call this script start, something more meaningful: browser-compile

webpack.config.js will be called by default so just put "webpack" in the script invocation

rricard · 2016-12-13T10:27:04Z

package.json

@@ -24,7 +26,8 @@
    "testonly": "mocha --reporter spec --full-trace ./dist/test/tests.js",
    "coverage": "node ./node_modules/istanbul/lib/cli.js cover _mocha -- --full-trace ./dist/test/tests.js",
    "postcoverage": "remap-istanbul --input coverage/coverage.raw.json --type lcovonly --output coverage/lcov.info",
-    "prepublish": "npm run compile"
+    "start": "webpack --config webpack.config.js",
+    "prepublish": "npm run compile && npm run start"


Instead of start, put browser-compile

rricard · 2016-12-13T10:28:15Z

webpack.config.js

+  output: {
+    path: __dirname + '/browser',
+    filename: 'bundle.js',
+    library: 'subscriptions_transport_ws'


Usually exported libs are in pascal case: 'SubscriptionsTransportWs', please do that!

rricard · 2016-12-13T10:29:20Z

webpack.config.js

@@ -0,0 +1,18 @@
+module.exports = {
+  context: __dirname + "/src",


Use path.join instead of a concatenation

rricard · 2016-12-13T10:29:24Z

webpack.config.js

+  context: __dirname + "/src",
+  entry: './index.ts',
+  output: {
+    path: __dirname + '/browser',


Use path.join instead of a concatenation

rricard · 2016-12-13T10:30:39Z

webpack.config.js

+    library: 'subscriptions_transport_ws'
+  },
+  resolve: {
+    extensions: ['', '.webpack.js', '.web.js', '.ts', '.js']


Why not just ['.ts', '.js', '.json'] ?

rricard · 2016-12-13T10:32:46Z

CHANGELOG.md

@@ -2,7 +2,7 @@

 ### vNEXT

- ...
+- Webpack configuration to export the generated code into a browser compatible bundle.


Nitpick: reference this PR number in the changelog

rricard · 2016-12-13T10:38:34Z

@helfer I'm concerned about the dependency on graphql-subscriptions wouldn't it be better to put it as a peer dependency? Normally, the browser bundle shouldn't touch it but even with that, having it as a dependency in node is not ideal... And having it as a peer would let us shim it in the webpack config.

rricard · 2016-12-13T10:44:14Z

@schrepfler On our side we should only expose the client module, so your entry point should only be: src/client.ts, that way we avoid the hassle with graphql-subscriptions

helfer · 2016-12-13T18:15:34Z

@rricard thanks a lot for tuning in here! ❤️ We can definitely (and probably should) make graphql-subscriptions a peer dependency.

rricard · 2016-12-13T18:33:32Z

package.json

@@ -10,8 +10,10 @@
  "dependencies": {
    "backo2": "^1.0.2",
    "graphql-subscriptions": "^0.2.0",


Ok, so while we're here, can you move this one in devDependencies and peerDependencies.

schrepfler · 2016-12-24T20:09:16Z

Went over the suggestions, only problem was with ['.ts', '.js', '.json'] I had to add '' to the list.

schrepfler · 2016-12-24T20:11:38Z

Ah, I also bumped a bit the libraries, but notably typed-graphql and graphql to get in line with the rest of the apollo stack. Anyhow, merry xMas!

schrepfler · 2016-12-24T20:25:54Z

Would the addition of graphql-subscriptions to peerDependencies mean that we would also need to create a browser bundle for it as well (and the other deps?)

Akryum · 2016-12-29T15:36:37Z

@rricard If a module is never imported in the code, webpack will not include it in the bundle. Declaring it as a peer dependency has no effect regarding webpack, but the externals config does (although not needed here, since graphql-subscriptions is never imported in the client code).

The only difference is that users of this package will have to manually install graphql-subscriptions if they want to use the server part of the package.

rricard

Seems better, I still need to test it before shipping it though...

schrepfler · 2017-01-06T22:12:44Z

@rricard can you please review the reversal of the change and comment on the runtime vs devDependencies wrt json and ts-loaders?

rricard

Seems good to me, I just need to check a few things first but once it's done, let's ship it!

schrepfler · 2017-01-31T19:35:53Z

Shall we try to ship it and then try to catch up with the rest of the apollo js stack? 😀🚀🛫🍻

Urigo · 2017-02-02T11:45:00Z

Maybe we can use the changes from this PR: #53 ? the package now compiles and create two entry points (one for client.ts and one for server.ts), and the client file does not have an external dependencies.
@rricard , maybe we can use this to avoid two compilations?

schrepfler · 2017-02-02T16:03:36Z

Hi @Urigo, not sure if I understand, are you saying #53 can deprecate this PR?
The rationale of #48 is more to create a browser bundle which can be directly used via unpkg.com once the library gets published onto npm.

Urigo · 2017-02-02T20:07:18Z

@schrepfler I meant that instead of creating a new Webpack config file and compile the package for browser, we might be able to do the same bundle process from the compiled files, since they're no longer coupled.
Also, can you please rebase this PR? thanks.

…et as umd

…s-transport-ws

schrepfler · 2017-02-02T22:35:22Z

Not sure what's going on, the rebase went into a loop, so tried to do the merge manually.

schrepfler · 2017-02-02T22:54:45Z

Somehow the new versions popped back into package.json. Hopefully it's good now.

Urigo · 2017-02-03T08:37:08Z

@schrepfler , thanks for the rebase. What about using the compiled client file, and then just bundling it?

schrepfler · 2017-02-03T18:43:18Z

I think this is the case, the only thing webpack is bundling is the client.ts? https://github.com/apollographql/subscriptions-transport-ws/pull/48/files#diff-11e9f7f953edc64ba14b0cc350ae7b9d

schrepfler · 2017-02-03T20:04:34Z

Also, please consider this has been submitted on the 15th November 2016 and JS is not my main skill, can we wrap it up? I don't mind if you use mine bundle or if you have your bundle.
All it has to provide is:

the package.json browser element pointing to the right bundle and
that bundle needs to be consumable directly in a browser context (so I guess no requires.) This bundle needs to target the browser, not node.
bonus: also export the helper fn? (I didn't do anything around that, personally I'd move it into the client bundle)

Urigo · 2017-02-05T12:48:42Z

@schrepfler sorry it took a long time, the other PR was pretty big and I was working on it while implementing subscriptions on one of our internal apps.
So about your last comment, the first and last points should be done with the new version.
So now what's left is just to expose the compiled client code without require.
You can update this PR or create another one

schrepfler · 2017-02-05T12:52:21Z

I think this is the case, I wrap client.ts which is then exposed as SubscriptionsTransportWs in the global scope if added to the page. I think

schrepfler · 2017-02-06T15:25:18Z

I just realised you're the same guy from the JSJabber podcast episode I just listened, nice one! :D

schrepfler · 2017-02-13T15:38:12Z

Any feedback in order to understand further course of actions?

schrepfler · 2017-02-19T19:16:45Z

I'll close this PR and start over.

Urigo · 2017-02-21T16:09:06Z

thanks again @schrepfler , linking the merged one if someone will stumble upon this one - #77

schrepfler added 3 commits November 15, 2016 18:41

Adding webpack config to create bundle

ea9763c

Adding browser bundle declaration in package.json

243d810

Describing feature to changelog

10ac03a

Adding reference to webpack in package.json

4c9b4ff

schrepfler mentioned this pull request Nov 17, 2016

Provide a browser bundle suitable for direct browser consumption #47

Closed

schrepfler added 4 commits November 18, 2016 19:53

Adding webpack step to prepublish and declaring library name and targ…

30a1d44

…et as umd

Change library export as graphql-subscriptions

e0dd42e

As the name is actually an existing artifact, changed to subscription…

3195c4a

…s-transport-ws

Adding config which exposes the client as a global.

ee4b77f

rricard suggested changes Dec 13, 2016

View reviewed changes

rricard reviewed Dec 13, 2016

View reviewed changes

Taking care of PR review comments.

c0939a8

rricard approved these changes Dec 29, 2016

View reviewed changes

rricard approved these changes Jan 7, 2017

View reviewed changes

schrepfler added 10 commits February 2, 2017 22:51

Adding webpack config to create bundle

815d97e

Adding browser bundle declaration in package.json

8b6fd90

Describing feature to changelog

b001028

Adding reference to webpack in package.json

c2e7a47

Adding webpack step to prepublish and declaring library name and targ…

c0604b9

…et as umd

Change library export as graphql-subscriptions

ad34073

As the name is actually an existing artifact, changed to subscription…

a481a65

…s-transport-ws

Adding config which exposes the client as a global.

af2ffa0

Rebased

5fc3497

Tried rebase, had to modify package.json as was getting in loop

90a160b

Reverting to master dependencies

e1d68de

schrepfler closed this Feb 19, 2017

schrepfler mentioned this pull request Feb 22, 2017

Adding package.json browser attribute pointing towards browser bundle… apollographql/apollo-client#1318

Closed

7 tasks

		@@ -0,0 +1,18 @@
		module.exports = {
		context: __dirname + "/src",

Module browser bundle configuration using webpack #48

Module browser bundle configuration using webpack #48

Conversation

schrepfler commented Nov 15, 2016 • edited by rricard Loading

apollo-cla commented Nov 15, 2016

schrepfler commented Nov 15, 2016

schrepfler commented Nov 16, 2016

schrepfler commented Nov 18, 2016

schrepfler commented Nov 20, 2016

schrepfler commented Nov 20, 2016

schrepfler commented Dec 3, 2016

helfer commented Dec 13, 2016

rricard commented Dec 13, 2016

rricard left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rricard commented Dec 13, 2016 • edited Loading

rricard commented Dec 13, 2016

helfer commented Dec 13, 2016

Choose a reason for hiding this comment

schrepfler commented Dec 24, 2016 • edited Loading

schrepfler commented Dec 24, 2016

schrepfler commented Dec 24, 2016

Akryum commented Dec 29, 2016 • edited Loading

rricard left a comment

Choose a reason for hiding this comment

schrepfler commented Jan 6, 2017

rricard left a comment

Choose a reason for hiding this comment

schrepfler commented Jan 31, 2017

Urigo commented Feb 2, 2017

schrepfler commented Feb 2, 2017

Urigo commented Feb 2, 2017

schrepfler commented Feb 2, 2017

schrepfler commented Feb 2, 2017

Urigo commented Feb 3, 2017

schrepfler commented Feb 3, 2017

schrepfler commented Feb 3, 2017 • edited Loading

Urigo commented Feb 5, 2017

schrepfler commented Feb 5, 2017

schrepfler commented Feb 6, 2017

schrepfler commented Feb 13, 2017

schrepfler commented Feb 19, 2017

Urigo commented Feb 21, 2017

schrepfler commented Nov 15, 2016 •

edited by rricard

Loading

rricard left a comment •

edited

Loading

rricard commented Dec 13, 2016 •

edited

Loading

schrepfler commented Dec 24, 2016 •

edited

Loading

Akryum commented Dec 29, 2016 •

edited

Loading

schrepfler commented Feb 3, 2017 •

edited

Loading