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

Bundle script instead of curl #851

Merged
merged 2 commits into from Apr 21, 2015

Conversation

Projects
None yet
9 participants
@arthuralee
Contributor

arthuralee commented Apr 15, 2015

This is an initial stab at creating a node-based bundling script so that you don't have to manually run curl. My idea was to have it as a command under react-native. It allows command line flags for dev and minify. It still requires the server to be up, but I hope to detect that and maybe automatically run the server in the future.

Wondering if this is a good idea and if so, the right approach.

@drkibitz

This comment has been minimized.

Show comment
Hide comment
@drkibitz

drkibitz Apr 16, 2015

Contributor

Suggestion:

http.get(options, function(res) {
    var stream = fs.createWriteStream(OUT_PATH);
    res.pipe(stream);
});
Contributor

drkibitz commented Apr 16, 2015

Suggestion:

http.get(options, function(res) {
    var stream = fs.createWriteStream(OUT_PATH);
    res.pipe(stream);
});
@arthuralee

This comment has been minimized.

Show comment
Hide comment
@arthuralee

arthuralee Apr 17, 2015

Contributor

Thanks @drkibitz, took your suggestion.

Contributor

arthuralee commented Apr 17, 2015

Thanks @drkibitz, took your suggestion.

Show outdated Hide outdated local-cli/cli.js
@@ -14,7 +15,8 @@ function printUsage() {
'',
'Commands:',
' start: starts the webserver',
' install: installs npm react components'
' install: installs npm react components',
' build: builds the javascript bundle for offline use'

This comment has been minimized.

@frantic

frantic Apr 17, 2015

Contributor

did you mean bundle?

@frantic

frantic Apr 17, 2015

Contributor

did you mean bundle?

This comment has been minimized.

@arthuralee

arthuralee Apr 17, 2015

Contributor

Good catch! I was actually debating build vs bundle

@arthuralee

arthuralee Apr 17, 2015

Contributor

Good catch! I was actually debating build vs bundle

This comment has been minimized.

@frantic

frantic Apr 17, 2015

Contributor

Let's keep it bundle, we are currently thinking about new build command that would compile ObjC code so you don't even have to run Xcode

@frantic

frantic Apr 17, 2015

Contributor

Let's keep it bundle, we are currently thinking about new build command that would compile ObjC code so you don't even have to run Xcode

@frantic

This comment has been minimized.

Show comment
Hide comment
@frantic

frantic Apr 17, 2015

Contributor

Thanks, that's really handy! We use something similar internally, but it's tied to our infra and build tools.

Actually react packager exposes API to create offline bundle without running the server, see buildPackageFromUrl. Would be awesome to use that instead of asking to run server manually. The only thing to keep in mind is ulimit - the OSX default 256 is way too low. @amasad - any thoughts on using graceful-fs?

And please update the comments in SampleApp's AppDelegate :)

Contributor

frantic commented Apr 17, 2015

Thanks, that's really handy! We use something similar internally, but it's tied to our infra and build tools.

Actually react packager exposes API to create offline bundle without running the server, see buildPackageFromUrl. Would be awesome to use that instead of asking to run server manually. The only thing to keep in mind is ulimit - the OSX default 256 is way too low. @amasad - any thoughts on using graceful-fs?

And please update the comments in SampleApp's AppDelegate :)

@amasad

This comment has been minimized.

Show comment
Hide comment
@amasad

amasad Apr 17, 2015

Contributor

@frantic I'll have to update multiple projects we depend on, and hopefully it doesn't cause any performance degradation (it will for people with lower ulimits). But I'm fine with that.

Contributor

amasad commented Apr 17, 2015

@frantic I'll have to update multiple projects we depend on, and hopefully it doesn't cause any performance degradation (it will for people with lower ulimits). But I'm fine with that.

@vjeux vjeux added the Needs Review label Apr 17, 2015

@arthuralee

This comment has been minimized.

Show comment
Hide comment
@arthuralee

arthuralee Apr 18, 2015

Contributor

I've updated the patch. It now uses ReactPackager and does not need the server running.

Some todos:

  • Take care of ulimit problem (Is this only a problem on some computers? Mine seems to be unlimited and I don't remember ever changing it)
  • Add a command to choose platform (blacklist)
  • Add a command line argument for output path
Contributor

arthuralee commented Apr 18, 2015

I've updated the patch. It now uses ReactPackager and does not need the server running.

Some todos:

  • Take care of ulimit problem (Is this only a problem on some computers? Mine seems to be unlimited and I don't remember ever changing it)
  • Add a command to choose platform (blacklist)
  • Add a command line argument for output path
@frantic

This comment has been minimized.

Show comment
Hide comment
@frantic

frantic Apr 19, 2015

Contributor

@arthuralee - great job! I don't think you should be solving ulimit issue in this PR, that's a more general problem that might be relevant to other tools as well. We should probably either switch to graceful-fs, wrap react-native-cli in a shell script that calls ulimit or use something like node-posix to call setrlimit directly.

I'll take a closer look and merge this in soon.

Contributor

frantic commented Apr 19, 2015

@arthuralee - great job! I don't think you should be solving ulimit issue in this PR, that's a more general problem that might be relevant to other tools as well. We should probably either switch to graceful-fs, wrap react-native-cli in a shell script that calls ulimit or use something like node-posix to call setrlimit directly.

I'll take a closer look and merge this in soon.

Show outdated Hide outdated local-cli/bundle.js
fs.writeFile(OUT_PATH, bundle.getSource({
inlineSourceMap: false,
minify: flags.minify
}), function() {

This comment has been minimized.

@frantic

frantic Apr 19, 2015

Contributor

Please handle the error here and show some helpful error message.

@frantic

frantic Apr 19, 2015

Contributor

Please handle the error here and show some helpful error message.

transformModulePath: require.resolve('../packager/transformer.js'),
assetRoots: [path.resolve(__dirname, '../../..')],
cacheVersion: '2',
blacklistRE: blacklist('ios')

This comment has been minimized.

@frantic

frantic Apr 19, 2015

Contributor

we can revisit this later when Android ships, no need for extra CLI option for now (you had this on your TODO comment)

@frantic

frantic Apr 19, 2015

Contributor

we can revisit this later when Android ships, no need for extra CLI option for now (you had this on your TODO comment)

@arthuralee

This comment has been minimized.

Show comment
Hide comment
@arthuralee

arthuralee Apr 19, 2015

Contributor

Thanks @frantic, sounds good. I've added error handling to fs.writeFile.

Maybe we should address the ulimit problem in a separate issue. I can open one if there isn't one yet.

Contributor

arthuralee commented Apr 19, 2015

Thanks @frantic, sounds good. I've added error handling to fs.writeFile.

Maybe we should address the ulimit problem in a separate issue. I can open one if there isn't one yet.

Show outdated Hide outdated local-cli/bundle.js
console.log('Building package...');
ReactPackager.buildPackageFromUrl(options, url)
.then(function(bundle) {

This comment has been minimized.

@frantic

frantic Apr 20, 2015

Contributor

nit: use done to make sure if ReactPackager.buildPackageFromUrl throws the error is reported.

@frantic

frantic Apr 20, 2015

Contributor

nit: use done to make sure if ReactPackager.buildPackageFromUrl throws the error is reported.

arthuralee added some commits Apr 15, 2015

Added bundle command using ReactPackager
Added bundle script
Pipe http response straight to file
Used ReactPackager directly, minor fixes
Added error handling to fs.writeFile
Changed .then to .done
@arthuralee

This comment has been minimized.

Show comment
Hide comment
@arthuralee

arthuralee Apr 21, 2015

Contributor

Makes sense, updated. I also updated the website docs.

Contributor

arthuralee commented Apr 21, 2015

Makes sense, updated. I also updated the website docs.

frantic added a commit that referenced this pull request Apr 21, 2015

Merge pull request #851 from arthuralee/master
Bundle script instead of curl

@frantic frantic merged commit 4792707 into facebook:master Apr 21, 2015

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@frantic

This comment has been minimized.

Show comment
Hide comment
@frantic

frantic Apr 21, 2015

Contributor

Awesome thanks! If you are willing to make this even better, here is a suggestion - add a step to end-to-end tests that calls react-native bundle and makes sure iOS/main.jsbundle got updated (by checking file size or looking for a substring in the file)

Contributor

frantic commented Apr 21, 2015

Awesome thanks! If you are willing to make this even better, here is a suggestion - add a step to end-to-end tests that calls react-native bundle and makes sure iOS/main.jsbundle got updated (by checking file size or looking for a substring in the file)

@arthuralee

This comment has been minimized.

Show comment
Hide comment
@arthuralee

arthuralee Apr 21, 2015

Contributor

Great to see this merged! Will look into e2e tests. :)

Contributor

arthuralee commented Apr 21, 2015

Great to see this merged! Will look into e2e tests. :)

@amasad

This comment has been minimized.

Show comment
Hide comment
@amasad

amasad Apr 23, 2015

Contributor

👯

Contributor

amasad commented Apr 23, 2015

👯

@tom76kimo

This comment has been minimized.

Show comment
Hide comment
@tom76kimo

tom76kimo Apr 25, 2015

Nice work! When will this be released? Just noticed the live Docs has updated but the cli tool hasn't. May introduce confusions. 😄

Nice work! When will this be released? Just noticed the live Docs has updated but the cli tool hasn't. May introduce confusions. 😄

@arthuralee

This comment has been minimized.

Show comment
Hide comment
@arthuralee

arthuralee Apr 25, 2015

Contributor

Good point. It's already been merged so its available on master, but not on @latest/@0.4.0 which is what people will get over npm install. Maybe at some point we'll need to versioning in the documentation...

Contributor

arthuralee commented Apr 25, 2015

Good point. It's already been merged so its available on master, but not on @latest/@0.4.0 which is what people will get over npm install. Maybe at some point we'll need to versioning in the documentation...

@romanonthego

This comment has been minimized.

Show comment
Hide comment
@romanonthego

romanonthego Apr 29, 2015

does bundle command included correctly in 0.4.1?
i've recieve error

ZapCourier [master●] % react-native bundle
Building package...

/Users/romanonthego/Code/ZapCourier/node_modules/react-native/node_modules/bluebird/js/main/promise.js:626
            throw e;
                  ^
Error: EMFILE, open '/Users/romanonthego/Code/ZapCourier/node_modules/react-native/node_modules/react-tools/src/browser/ui/dom/ViewportMetrics.js'

every time.
npm start + curl still works fine.

on same notice - why react 0.4.0 and 0.4.1 does not marked as latest release? is it considerate unstable or is it just a github problem (npm @latest installs 0.4.1)?

does bundle command included correctly in 0.4.1?
i've recieve error

ZapCourier [master●] % react-native bundle
Building package...

/Users/romanonthego/Code/ZapCourier/node_modules/react-native/node_modules/bluebird/js/main/promise.js:626
            throw e;
                  ^
Error: EMFILE, open '/Users/romanonthego/Code/ZapCourier/node_modules/react-native/node_modules/react-tools/src/browser/ui/dom/ViewportMetrics.js'

every time.
npm start + curl still works fine.

on same notice - why react 0.4.0 and 0.4.1 does not marked as latest release? is it considerate unstable or is it just a github problem (npm @latest installs 0.4.1)?

@arthuralee

This comment has been minimized.

Show comment
Hide comment
@arthuralee

arthuralee Apr 29, 2015

Contributor

Thanks @romanonthego. I will look into this today. There have been a lot of new changes since this has been merged, so it might have broken something. Looks like I should've gone into e2e tests earlier! In the meantime, feel free to create an open issue for this.

As far as I know, @latest installs the latest tagged version, which is currently 0.4.1.

Contributor

arthuralee commented Apr 29, 2015

Thanks @romanonthego. I will look into this today. There have been a lot of new changes since this has been merged, so it might have broken something. Looks like I should've gone into e2e tests earlier! In the meantime, feel free to create an open issue for this.

As far as I know, @latest installs the latest tagged version, which is currently 0.4.1.

@arthuralee

This comment has been minimized.

Show comment
Hide comment
@arthuralee

arthuralee Apr 29, 2015

Contributor

@romanonthego I could not repro the error. I tested both an empty project and an existing project. To further verify this, the file in question (/node_modules/react-native/node_modules/react-tools/src/browser/ui/dom/ViewportMetrics.js) seems to be bundled correctly and in main.jsbundle. Could you perhaps produce a minimal example of the bug?

Contributor

arthuralee commented Apr 29, 2015

@romanonthego I could not repro the error. I tested both an empty project and an existing project. To further verify this, the file in question (/node_modules/react-native/node_modules/react-tools/src/browser/ui/dom/ViewportMetrics.js) seems to be bundled correctly and in main.jsbundle. Could you perhaps produce a minimal example of the bug?

@frantic

This comment has been minimized.

Show comment
Hide comment
@frantic

frantic Apr 30, 2015

Contributor

@romanonthego - the problem is caused by too many files and low open files limit per process. Run ulimit -n 2560 in your terminal before running react-native bundle.

@arthuralee, it's hard to repro because different environments might have different ulimits, also some users will have more js files in node_modules and will hit the bug, wheres vanilla react-native can fit in standard 256 limit.

Contributor

frantic commented Apr 30, 2015

@romanonthego - the problem is caused by too many files and low open files limit per process. Run ulimit -n 2560 in your terminal before running react-native bundle.

@arthuralee, it's hard to repro because different environments might have different ulimits, also some users will have more js files in node_modules and will hit the bug, wheres vanilla react-native can fit in standard 256 limit.

@romanonthego

This comment has been minimized.

Show comment
Hide comment
@romanonthego

romanonthego Apr 30, 2015

I've just cleaned npm cache and reinstall all global packages (react-native and react-native-cli including).
Bug is gone, all works like a charm.

@frantic i've used this fix and it didn't help at the moment (I've encountered this bug before, although it was in form Error: EMFILE, too many open files and not Error: EMFILE, open #{path}).

I've just cleaned npm cache and reinstall all global packages (react-native and react-native-cli including).
Bug is gone, all works like a charm.

@frantic i've used this fix and it didn't help at the moment (I've encountered this bug before, although it was in form Error: EMFILE, too many open files and not Error: EMFILE, open #{path}).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment