[GPT-432] Package server webpack #346

nickbalestra · 2017-01-10T17:40:28Z

matthewdavidson · 2017-01-11T09:26:13Z

Looking good so far @nickbalestra! I know this is still WIP but some quick comments on the webpack config:

I suspect we will need libraryTarget: 'commonjs2' specified in the output section, otherwise the oc-registry will have nothing to consume
In order to avoid packaging up npm dependencies and delegate resolving to the oc-registry, we should configure something along these lines:externals: /^[a-z@][a-z\-\/0-9]+$/, (regex not tested thoroughly).

nickbalestra · 2017-01-11T11:49:15Z

@matthewdavidson yes, thanks 💯 for reminding the commonjs2 target !
For the externals your regex seems fine, perhaps a simple alternative could be just /^[^\.\/]+$/ ? But I guess some more name-validation as you are doing won't hurt 💅

nickbalestra · 2017-01-11T13:09:42Z

@matthewdavidson added libraryTarget and externals. @matteofigus need to add some integration tests for them, and then we'll probably be ready for a first review

nickbalestra · 2017-01-12T16:44:08Z

Quick update you on the status of this pr:

Need to investigate why some tests fails on windows.
Add unit tests (all the integration tests are in place).
Clean legacy code

nickbalestra · 2017-01-13T17:55:25Z

@matteofigus @matthewdavidson should be ready to go for a review.

nickbalestra · 2017-01-13T18:03:24Z

test/unit/cli-domain-package-server-script.js

+      var regex = handler[1];
+      it('should match npm module names', function() {
+        expect(regex.test('lodash')).to.be.true;
+        expect(regex.test('lodash/fp/curryN')).to.be.true;


@matthewdavidson that helped me fine an exception to our regularExpression for the webpack externals:
A relative path from an npm module can in fact contain upperCase letters

👍 nice catch

matthewdavidson

We've tried this out on one of our components and it all works as expected.

i.e. listed "oc": "opentable/oc#05bea7c48719824248b0777dcb25a67491eeccf4" in package.json and modified server.js to use es6 & local requires.

🎉

matthewdavidson · 2017-01-18T04:20:18Z

src/cli/domain/package-server-script/bundle/config/index.js

+      path: '/build',
+      filename: params.fileName,
+      libraryTarget: 'commonjs2'
+    },


Should be be setting target: 'node', here? (see https://webpack.github.io/docs/configuration.html#target)

good catch!

nickbalestra · 2017-01-18T07:10:42Z

Thanks Matthew! Will do some more extensive tests today on some of our most complex components as well. Then we should be ready to ship

…

On 18 Jan 2017, at 05:27, Matthew Davidson ***@***.***> wrote: @matthewdavidson commented on this pull request. We've tried this out on one of our components and it all works as expected. i.e. listed "oc": "opentable/oc#05bea7c48719824248b0777dcb25a67491eeccf4" in package.json and modified server.js to use es6 & local requires. 🎉 In src/cli/domain/package-server-script/bundle/config/index.js: > +/*jshint camelcase:false */ +'use strict'; + +var webpack = require('webpack'); +var path = require('path'); +var wrapLoops = require('./wrapLoops'); +var externalDependenciesHandlers = require('./externalDependenciesHandlers'); + +module.exports = function webpackConfigGenerator(params){ + return { + entry: params.dataPath, + output: { + path: '/build', + filename: params.fileName, + libraryTarget: 'commonjs2' + }, Should be be setting target: 'node', here? (see https://webpack.github.io/docs/configuration.html#target) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

chriscartlidge · 2017-01-18T10:27:15Z

package.json

@@ -78,6 +83,7 @@
    "targz": "1.0.1",
    "uglify-js": "2.6.4",
    "underscore": "1.8.3",
-    "watch": "0.19.1"
+    "watch": "0.19.1",
+    "webpack": "1.14.0"


Worth upgrading to 2.0 since the new hot sauce has been released... Maybe too soon?

If it's out of beta I 👍 on this, unless it requires a significant amount of work so that we want to keep it to the next PR

Yes, and thats great news 🎉
Although, until we finally drop ourself support for node 0.10&co I will suggest we stick to v1. I'll prepare a v2 upgrade on a separate PR, what you think?

ok @nickbalestra

chriscartlidge · 2017-01-18T10:30:28Z

src/cli/domain/package-server-script/bundle/config/index.js

+              'presets': [
+                [require.resolve('babel-preset-env'), {
+                  'targets': {
+                    'node': 0.10


I know why we are doing this but is there a plan to move away from 0.10... LTS has even ran out on it: https://github.com/nodejs/LTS

I know. This should be a short term pain though. I was planning to update this with the wepack2 upgrade as by tame we'll merge that mean we don't have any more 0.10&co to support. After that we my want to consider adding some global config to oc where you could set that target info there. Thoughts?

I think it's fine if we go for node 4 with this. Node 0.10 is supported but we can remove support as soon as we merge this as a subsequent PR ;)

…tion test

matteofigus

A couple of little changes and then I think we're ready to go

matteofigus · 2017-01-19T16:50:15Z

src/cli/domain/package-server-script/compress.js

-    return uglifyJs.minify(code, { fromString: true }).code;
-  } catch (e){
-    if(!!e.line && !!e.col){
-      throw new Error(format(strings.errors.cli.SERVERJS_PARSING_ERROR, fileName, e.line, e.col, e.message));


Let's remove this SERVERJS_PARSING_ERROR

matteofigus · 2017-01-19T16:50:34Z

src/cli/domain/package-server-script/getLocalDependencies.js

-  if(ext === ''){
-    required += '.json';
-  } else if(ext !== '.json'){
-    throw new Error(strings.errors.cli.SERVERJS_REQUIRE_JS_NOT_ALLOWED);


same for SERVERJS_REQUIRE_JS_NOT_ALLOWED

matteofigus · 2017-01-19T16:50:45Z

src/cli/domain/package-server-script/getLocalDependencies.js

-
-  var requiredPath = path.resolve(componentPath, required);
-  if(!fs.existsSync(requiredPath)){
-    throw new Error(format(strings.errors.cli.SERVERJS_REQUIRE_JSON_NOT_FOUND, required));


here too SERVERJS_REQUIRE_JSON_NOT_FOUND

matteofigus · 2017-01-19T16:50:54Z

src/cli/domain/package-server-script/bundle/index.js

+  compiler.outputFileSystem = memoryFs;
+
+  compiler.run(function(error, stats){
+    var sofError;


matteofigus · 2017-01-19T17:00:06Z

test/integration/cli-domain-package-server-script/cli-domain-package-server-script.js

+        fs.writeFileSync(path.resolve(componentPath, 'user.json'), jsonContent);
+        fs.writeFileSync(path.resolve(componentPath, serverName), serverContent);
+        done();
+      });


we should add an afterEach for ensuring that other requires to same (changed?) file don't get the cached version

require.cache[path.resolve(publishPath, res.src)] = null;

Sweet one, thanks for that!!! 🥇

matteofigus · 2017-01-19T17:02:31Z

test/integration/cli-domain-package-server-script/cli-domain-package-server-script.js

+        );
+      });
+
+      describe('end required depenencies is not present in the package.json', function(){


typo ("and" and "depenencies")

matteofigus · 2017-01-19T17:04:48Z

test/integration/cli-domain-package-server-script/cli-domain-package-server-script.js

+        done();
+      });
+
+      it('should throw an error when the dependency is not present in the package.json', function(done){


to keep consistency, can we do a describe for "when the dependency is not present in the package.json" and then a nested it for "should throw an error"?

matteofigus · 2017-01-19T17:05:54Z

test/integration/cli-domain-package-server-script/cli-domain-package-server-script.js

+            }
+            try {
+              var name = 'John';
+              var bundle = require(path.resolve(publishPath, res.src));


same as before for an afterEach that contains the require cache invalidation

matteofigus · 2017-01-19T17:17:05Z

test/unit/cli-domain-package-server-script.js

      });

-      it('should wrap the do loop with an iterator limit (and convert it to a for loop)', function(){
-        expect(fsMock.writeFile.firstCall.args[1]).to.contain('for(var i=1e9;;){if(i<=0)throw new Error(\"loop exceeded maximum allowed iterations\");t=342,i--}');
+      it('should return a proper confifuration options for webpack', function(){


"confifuration" typo

matteofigus · 2017-01-19T17:18:02Z

test/unit/cli-domain-package-server-script.js

+        webpack: { stats: 'none' },
+        dependencies: {},
+        fileName: 'server.js',
+        dataPath: '/Users/nbalestra/dev/oc/test/integration/cli-domain-package-server-script/component/server.js'


I think it's better if we do a readable example like /path/to/server.js

matteofigus · 2017-01-20T12:43:18Z

nickbalestra added cli enhancement wip labels Jan 10, 2017

matteofigus changed the title ~~Package server webpack~~ [don't merge yet] Package server webpack Jan 10, 2017

nickbalestra force-pushed the package-server-webpack branch from 0ab4fc1 to 14acc12 Compare January 11, 2017 04:36

nickbalestra force-pushed the package-server-webpack branch 2 times, most recently from 2af7991 to cde708d Compare January 12, 2017 16:39

nickbalestra force-pushed the package-server-webpack branch from c7eddb7 to 14d4787 Compare January 13, 2017 17:52

nickbalestra mentioned this pull request Jan 13, 2017

Revisit dev watcher-logic #351

Closed

nickbalestra commented Jan 13, 2017

View reviewed changes

nickbalestra force-pushed the package-server-webpack branch 2 times, most recently from a31e10a to 843b471 Compare January 13, 2017 18:15

nickbalestra requested review from chriscartlidge, matthewdavidson and matteofigus January 13, 2017 18:22

nickbalestra changed the title ~~[don't merge yet] Package server webpack~~ Package server webpack Jan 13, 2017

nickbalestra force-pushed the package-server-webpack branch from 843b471 to 3e83569 Compare January 13, 2017 18:45

nickbalestra changed the title ~~Package server webpack~~ [GPT-432] Package server webpack Jan 16, 2017

nickbalestra force-pushed the package-server-webpack branch from b224c5f to eae270a Compare January 17, 2017 09:00

matthewdavidson reviewed Jan 18, 2017

View reviewed changes

nickbalestra force-pushed the package-server-webpack branch 2 times, most recently from a10342b to fd38f7e Compare January 18, 2017 10:26

chriscartlidge reviewed Jan 18, 2017

View reviewed changes

Nick Balestra added 18 commits January 18, 2017 17:24

tests

89aafd3

initial test

1bb52c7

testing compiler errors

fd0c275

moved integration test to /integration

17ef3b3

drop rimraf -> fs-extra

afdb51e

integration test wip

c312a54

es6 example integration test

d97575c

cleanup

494a8b1

added config module

0b491bb

libraryTarget=commonjs2, externals=regex, added json-loader & integra…

4939afd

…tion test

wrap while/do/for;; loops with an iterator limit

818f58a

structure/internaAPIi refactor

8944a47

integration tests

9b3fefe

unit tests

366200e

resolveLoader

19477b3

added target node for the webpack config

2a9cd22

tests cleanup

41944ce

rebase

dbf4714

nickbalestra force-pushed the package-server-webpack branch from 956b6e4 to dbf4714 Compare January 18, 2017 17:24

nickbalestra mentioned this pull request Jan 19, 2017

Webpack 2 #357

Merged

1 task

matteofigus requested changes Jan 19, 2017

View reviewed changes

review-fixes

34dc1ce

matteofigus approved these changes Jan 20, 2017

View reviewed changes

matteofigus merged commit 81cdaed into master Jan 20, 2017

matteofigus deleted the package-server-webpack branch January 20, 2017 11:36

This was referenced Jan 20, 2017

ES2015 support in server.js #339

Closed

Consider support for local requires in server.js #340

Closed

This was referenced Jan 20, 2017

ES6 component fails to compile when object properties shorthand #358

Closed

Publish bugfix #365

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GPT-432] Package server webpack #346

[GPT-432] Package server webpack #346

nickbalestra commented Jan 10, 2017 •

edited

Loading

matthewdavidson commented Jan 11, 2017

nickbalestra commented Jan 11, 2017 •

edited

Loading

nickbalestra commented Jan 11, 2017

nickbalestra commented Jan 12, 2017

nickbalestra commented Jan 13, 2017 •

edited

Loading

nickbalestra Jan 13, 2017

matthewdavidson Jan 18, 2017

matthewdavidson left a comment

matthewdavidson Jan 18, 2017

nickbalestra Jan 18, 2017

nickbalestra commented Jan 18, 2017 via email

chriscartlidge Jan 18, 2017

matteofigus Jan 18, 2017

nickbalestra Jan 18, 2017

matteofigus Jan 18, 2017

chriscartlidge Jan 18, 2017

nickbalestra Jan 18, 2017

matteofigus Jan 18, 2017

matteofigus left a comment

matteofigus Jan 19, 2017

matteofigus Jan 19, 2017

matteofigus Jan 19, 2017

matteofigus Jan 19, 2017

matteofigus Jan 19, 2017

nickbalestra Jan 20, 2017 •

edited

Loading

matteofigus Jan 19, 2017

nickbalestra Jan 20, 2017

matteofigus Jan 19, 2017

matteofigus Jan 19, 2017

matteofigus Jan 19, 2017

matteofigus Jan 19, 2017

nickbalestra Jan 20, 2017

matteofigus commented Jan 20, 2017

[GPT-432] Package server webpack #346

[GPT-432] Package server webpack #346

Conversation

nickbalestra commented Jan 10, 2017 • edited Loading

matthewdavidson commented Jan 11, 2017

nickbalestra commented Jan 11, 2017 • edited Loading

nickbalestra commented Jan 11, 2017

nickbalestra commented Jan 12, 2017

nickbalestra commented Jan 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewdavidson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickbalestra commented Jan 18, 2017 via email

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

matteofigus left a 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

nickbalestra Jan 20, 2017 • 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

matteofigus commented Jan 20, 2017

nickbalestra commented Jan 10, 2017 •

edited

Loading

nickbalestra commented Jan 11, 2017 •

edited

Loading

nickbalestra commented Jan 13, 2017 •

edited

Loading

nickbalestra Jan 20, 2017 •

edited

Loading