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

[WIP] Webpack support #1117

Closed
wants to merge 6 commits into from
Closed

[WIP] Webpack support #1117

wants to merge 6 commits into from

Conversation

chrisradek
Copy link
Contributor

/cc @LiuJoyceC

This PR adds support for webpack out of the box, and maintains browserify support.
This will allow webpack to pull in the SDK with all default services, assuming no custom webpack config. There is a way to specify services with custom config but it is still a bit messy, I want to clean that up.

Tested with a personalized starter app as well as a create-react-app project.

Still a WIP. Need to test in IE, add documentation, and update tooling.

Address #603

@coveralls
Copy link

coveralls commented Aug 30, 2016

Coverage Status

Coverage increased (+0.03%) to 88.931% when pulling dcc6109 on chrisradek:webpack into 7e3c279 on aws:master.

@chrisradek chrisradek mentioned this pull request Aug 30, 2016
if (typeof process === 'undefined') {
process = {
browser: true
};

Choose a reason for hiding this comment

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

Should be this.process = ...?

@simonbuchan
Copy link

Nice!

Initial Qs:

  • Can dist-tools/service-loader reuse the existing dist-tools? Seems we want to avoid duplication, and if I'm using those, I expect others are too, so they can't be removed.
  • Could /lib/browser_services be something like /svc/all and require all /svc/{id}?

@simonbuchan
Copy link

simonbuchan commented Aug 30, 2016

To be a bit clearer on the last point, to do something like what core.js do to support partial use, so you can use only import S3 from 'aws-sdk/svc/s3'; and get just S3 in your build:

// aws-sdk/svc/s3.js
// generated by aws-sdk/dist-tools/service-loader.js
var AWS = require('../lib/aws'); // will be '../lib/browser.js' in browserify, webpack, etc...?

AWS.apiLoader.services['s3'] = {};
AWS.S3 = AWS.Service.defineService('s3', [ '2006-03-01' ]);
require('./services/s3');
AWS.apiLoader.services['s3']['2006-03-01'] = require('../apis/s3-2006-03-01.min');
AWS.apiLoader.services['s3']['2006-03-01'].paginators = require('../apis/s3-2006-03-01.paginators').pagination;
AWS.apiLoader.services['s3']['2006-03-01'].waiters = require('../apis/s3-2006-03-01.waiters2').waiters;

module.exports = AWS.S3;

It... looks like it should work?

@chrisradek
Copy link
Contributor Author

@simonbuchan
Thanks for posting!
Yes, I should be able to merge service-loader into service-collector.

I think we can do something similar to your example. I'll take a look into that today.

@@ -1,3 +1,11 @@
var util = require('./util');
Copy link
Contributor

Choose a reason for hiding this comment

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

@chrisradek Do you think it might be worthwhile adding a buffer polyfill as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely. Webpack actually does polyfill buffer, but I like taking control of which buffer polyfill to use back to us (makes updating when bugs are fixed easier), and to make supporting other tools easier.

…ing the browser version of the SDK.

Updated browser-builder cli to handle changes made to how the SDK loads browser services.

Added crypto-browserify and buffer as dependencies to allow us greater control over deps, and to help compatibility with 3rd party tools.

Added script to generate browser service files.
@chrisradek
Copy link
Contributor Author

I added some changes to the PR.

With these changes, users can require individual services and webpack/browserify will only pull those in.

In the user's code, they would do something like this to require S3:

var S3 = require('aws-sdk/browser/s3');

Access to the AWS namespace (useful for setting config across multiple service clients) is available like so:

var AWS = require('aws-sdk/browser/global');

aws-sdk/browser/all can be required to get all services.

Of course, calling var AWS = require('aws-sdk') will continue to work as it does today, returning the AWS namespace with the default service clients on it.

Today I'll be working on some documentation and some more testing. The PR shows a lot of file changes but this is largely due to generating the individual services so that they can be required individually.

@AdityaManohar
Copy link
Contributor

AdityaManohar commented Sep 1, 2016

Can this be changed to just

var S3 = require('aws-sdk/s3');
var AWS = require('aws-sdk/global');

Why do we need the browser/ prefix? Browserifying for Node seems to be a legitimate use case. See #696

@chrisradek
Copy link
Contributor Author

@AdityaManohar
The way I have things currently set up, the stubs in aws-sdk/browser/* will also build the browser version of AWS, which means it'll be using JS crypto and buffer libraries, as well as xhr instead of https. I did this so that requiring a single service gives you everything you need (including the core of the SDK).

I chose the browser prefix in case we wanted to do the same for node.js. I don't believe dynamically choosing which AWS to load will work either, since tools will just look at what's required and pull that in, regardless of if it's actually used or not.

I'll have to look into that issue some more to see if what's here would work for that use case as well.

@AdityaManohar
Copy link
Contributor

AdityaManohar commented Sep 1, 2016

@chrisradek
Let me illustrate the use case I'm talking about:

// main.js in my npm module


// In the past I would do this
// var AWS = require('aws-sdk');

// This is the new proposal
var S3 = require('aws-sdk/browser/s3');

var headS3Object = function() {
  var s3 = new AWS.S3({region: 'us-west-2'});
  s3.headObject({/*params*/}, callback);
}

module.exports = headS3Object;

Using browserify:

$> browserify main.js -o bundle.js

This is use case that currently works with browserify, except for the selective service module inclusions.

Now I could also chose to use main.js as a node module.

From what you are saying, my understanding is that I cannot just take main.js as use it as a Node module because I'm specifically importing browser versions of the the service modules. Is that understanding correct?

@chrisradek
Copy link
Contributor Author

@AdityaManohar
That's correct. Essentially, the browser entry point would be loaded when requiring the service.

I might be able to create a node.js copy of the service stubs, and then in the package.json browser field map the node.js stubs to the browser ones. Then, if the user is using a tool that looks at the browser field (like webpack and browserify), the browser stubs would be used by default.

I'm open to any ideas on how else to accomplish this (besides having separate packages), and I can at least go down the above route to see how feasible that is.

var fs = require('fs');
var path = require('path');

var defaultServices = 'acm,apigateway,applicationautoscaling,autoscaling,cloudformation,cloudfront,cloudhsm,cloudtrail,cloudwatch,cloudwatchlogs,cloudwatchevents,codecommit,codedeploy,codepipeline,cognitoidentity,cognitoidentityserviceprovider,cognitosync,configservice,devicefarm,directconnect,dynamodb,dynamodbstreams,ec2,ecr,ecs,elasticache,elasticbeanstalk,elastictranscoder,elb,emr,firehose,gamelift,inspector,iotdata,kinesis,kms,lambda,marketplacecommerceanalytics,mobileanalytics,machinelearning,opsworks,rds,redshift,route53,route53domains,s3,ses,sns,sqs,ssm,storagegateway,sts,waf';
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of maintaining a separate list (I know this is what we currently do anyway), we would have fewer places to worry about making manual changes for service updates if we move this information into metadata.json, e.g.

{
    "acm": {
        "name": "ACM",
        "defaultService": true
    },
    ...
}

That way we can also get rid of the identical identical defaultServices lists in service-collector.js as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I like that idea.

@AdityaManohar
Copy link
Contributor

I'm open to any ideas on how else to accomplish this (besides having separate packages), and I can at least go down the above route to see how feasible that is.

I'll try to send you something soon.

IMO, it is a better user experience to not have to make the distinction between browser bundles and Node bundles upfront. The existing tooling ecosystem (Webpack, Browserify) largely supports writing once and running anywhere, and it would be nice if the SDK could support this out of the box as well.

@chrisradek
Copy link
Contributor Author

Closing in favor of #1123

@lock
Copy link

lock bot commented Sep 28, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@lock lock bot locked as resolved and limited conversation to collaborators Sep 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants