Add support for npm package #184

unDemian · 2018-03-27T17:36:52Z

Built on top of #183

Summary

This PR prepares happychat-client to be exposed as a npm package. Paths are resolved using babel-plugin-module-resolver and configuration json files are inlined using babel-plugin-inline-json.

Testing

Switch calypso to try/happychat-client branch (Use happychat-client npm package wp-calypso#23630)
Link this package to wp-calypso, by following these steps

run npm link in this folder
run npm link happychat-client in calypso folder

Run npm install in this branch
Run npm run dev:standalone in this repo
Grab your oauth token from a wpcom request made by the client (http://localhost:9000) it is under Authorization header after Bearer
Add the token to calypso https://github.com/Automattic/wp-calypso/pull/23630/files#diff-56de7193249f7b0107d6033c1a7737acR22
Run npm dev:npm
Access http://calypso.localhost:3000/me/chat
It should load the happychat-client iframe

lamosty

If I don't have development.local.json, it won't compile and complain: http://cld.wthms.co/K5ttNz. I thought having it is optional. Or not?

lamosty · 2018-03-28T02:16:53Z

What's the command npm dev:npm? There isn't one like that in wp-calypso. When I run npm start, it complains that "Module not found: Error: Can't resolve 'happychat-client'"

unDemian · 2018-03-28T10:13:06Z

If I don't have development.local.json, it won't compile and complain: http://cld.wthms.co/K5ttNz. I thought having it is optional. Or not?

Nice catch, it should be optional but webpack will fail at build time because it resolves the requires bypassing the try/catch. I added a postinstall script that creates an empty local config file if it is missing.

What's the command npm dev:npm? There isn't one like that in wp-calypso. When I run npm start, it complains that "Module not found: Error: Can't resolve 'happychat-client'"

dev:npm command will run a babel process with watch to compile the files into targets/npm and also run a webpack-dev-server that will serve the local css on port 9000.

Most likely the error appears because the happychat-client is not linked. The Calypso PR adds the happychat-client to the package.json however it is not published to npm which will make calypso npm install to fail. For development purposes you can link a local npm package to your project (calypso) by following step 2 in the description. That will basically create a symlink from calypso/node_modules/happychat-client to your local copy of this repo.

oandregal · 2018-04-01T18:34:14Z

package.json

    "start": "run-p dev:standalone",
    "dev:standalone": "run-p dev:standalone:*",
    "dev:standalone:js": "NODE_ENV=development webpack-dev-server --config webpack.standalone.config.js",
    "dev:standalone:css": "node-sass src/form.scss | postcss --config postcss.config.json -u autoprefixer -u postcss-custom-properties -o targets/standalone/happychat.css",
+    "dev:npm": "run-p dev:npm:*",
+    "dev:npm:js": "NODE_ENV=development ./node_modules/babel-cli/bin/babel.js --watch src --out-dir targets/npm",


isn't babel in node_modules/.bin/? If so, you can actually do NODE_ENV=development babel --watch src --out-dir targets/npm because the scripts will search in node_module/.bin path for "binaries".

oandregal · 2018-04-01T18:37:25Z

src/config/index.js

-		merge( configFile, localConfig );
-	} catch ( e ) {}
+	// check if we have local git ignored config and use it to override the development.json
+	const localConfig = require( 'src/config/development.local.json' );


Instead of creating a fake development.local.json file, wouldn't it be better to check whether it exists here?

oandregal · 2018-04-01T18:38:25Z

webpack.npm.config.js

+	case 'production':
+		config.plugins.push( new webpack.optimize.ModuleConcatenationPlugin() );
+		config.plugins.push( new UglifyJsPlugin() );
+		config.plugins.push( new LodashModuleReplacementPlugin( {


We no longer use the lodash-webpack-plugin.

oandregal · 2018-04-01T18:51:21Z

Tried and it works! 🎉

As far as I know, /me/chat was thought as an escape hatch for mobile users, so it used the whole "main content" space to display. By using the iframe as it is, that is no longer true. One potential solution for this is to expose the iframe height / width through the API. This will solve the /me/chat issue and will actually allow any consumer to declare the size they want Happychat to use. This is not hassle-free, though, and it raises some questions: for example, whether we should meet the minimum size requirements declared in the iframe -no matter what the consumer wants- as to prevent issues when the entry point is the contact form, not the chat form.

unDemian · 2018-04-02T07:38:58Z

As far as I know, /me/chat was thought as an escape hatch for mobile users, so it used the whole "main content" space to display.

That is correct but the Calypso branch was just an experiment, and for that me/chat was easier to refactor.

By using the iframe as it is, that is no longer true. One potential solution for this is to expose the iframe height / width through the API. This will solve the /me/chat issue and will actually allow any consumer to declare the size they want Happychat to use. This is not hassle-free, though, and it raises some questions: for example, whether we should meet the minimum size requirements declared in the iframe -no matter what the consumer wants- as to prevent issues when the entry point is the contact form, not the chat form.

Currently working on this :)

unDemian · 2018-04-02T07:39:15Z

Thanks for taking the time 🏅

unDemian added 8 commits March 27, 2018 17:10

Extract api and use different entry points

53f492d

Add api file and new entry point for dist target

e4e5bbc

Remove dist files from repo

ff1d9ff

Rename dist to browser

6e25b05

Add npm commands for dev and fix hotrealoading

1e163e5

Use local css and configs for development

3a3986c

Move css file to config

9571c89

Add support for npm package

ed90a85

unDemian added the [Status] Needs Review label Mar 27, 2018

unDemian self-assigned this Mar 27, 2018

unDemian requested review from oandregal and a team March 27, 2018 17:38

unDemian mentioned this pull request Mar 27, 2018

Create npm package out of client's api #169

Closed

unDemian and others added 3 commits March 27, 2018 20:47

Address feedback

845bcd9

Fix tests

3aae912

Update webpack.dist.config.js

fbfc9cf

lamosty reviewed Mar 28, 2018

View reviewed changes

unDemian added 3 commits March 28, 2018 12:05

Update package-lock to match master

e9934e2

Remove lodash-webpack-plugin

e3a1930

Address feedback

f0b492c

Add local config file on postinstall

6297c63

unDemian mentioned this pull request Mar 28, 2018

Add support for wpcom proxy auth #188

Merged

Initial jsdocs for eventAPI

036a38b

oandregal reviewed Apr 1, 2018

View reviewed changes

unDemian added 12 commits April 2, 2018 11:13

Fix merge conflicts

a8d1967

Fix merge conflicts

3a74514

Fix merge conflicts

237ec14

Remove dists

0c1a3bc

Fix merge conflicts

b6bb6d8

Fix commands

a45139b

Fix targets npm command

2790e04

Fix merge conflicts

595906e

Fix merge conflicts

e177c5f

Update standalone

ffeddf6

Fix conflicts

43cf670

Fix merge conflicts

5b3d71a

unDemian changed the base branch from add/css-file-config to master April 2, 2018 10:22

unDemian added 2 commits April 2, 2018 13:25

Fix merge conflicts

192cf16

Fix merge conflics

339ab33

unDemian merged commit aad8f66 into master Apr 2, 2018

unDemian removed the [Status] Needs Review label Apr 2, 2018

unDemian mentioned this pull request Apr 2, 2018

Minimize JS/CSS library bundle #10

Closed

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for npm package #184

Add support for npm package #184

unDemian commented Mar 27, 2018 •

edited

Loading

lamosty left a comment

lamosty commented Mar 28, 2018

unDemian commented Mar 28, 2018

oandregal Apr 1, 2018

oandregal Apr 1, 2018

oandregal Apr 1, 2018

oandregal commented Apr 1, 2018

unDemian commented Apr 2, 2018

unDemian commented Apr 2, 2018

Add support for npm package #184

Add support for npm package #184

Conversation

unDemian commented Mar 27, 2018 • edited Loading

Summary

Testing

lamosty left a comment

Choose a reason for hiding this comment

lamosty commented Mar 28, 2018

unDemian commented Mar 28, 2018

oandregal Apr 1, 2018

Choose a reason for hiding this comment

oandregal Apr 1, 2018

Choose a reason for hiding this comment

oandregal Apr 1, 2018

Choose a reason for hiding this comment

oandregal commented Apr 1, 2018

unDemian commented Apr 2, 2018

unDemian commented Apr 2, 2018

unDemian commented Mar 27, 2018 •

edited

Loading