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

reduce npm package file size 95% #309

Closed
wants to merge 1 commit into from

Conversation

shibukawa
Copy link
Contributor

Remove files they seem not to be needed for npm package user.
File size becomes 5MB to 250KB.

npm uses node-tar package that is a pure JavaScript tar archiver. It is extremely slower than native tar command. Smaller file size is better for users.

@kazuho
Copy link
Member

kazuho commented Apr 2, 2014

Thank you for the PR. The approach looks logical to me. Some comments:

  • are we going to unbundle examples?
    • if yes, we should unbundle example/ as well
    • if no, then we would not unbundle web/
  • profiler server is implemented within web/server.js and we should find a way to keep it within the release bundle if we are to strip them out

@gfx any ideas?

@shibukawa
Copy link
Contributor Author

I noticed doc is not bundled now. I think doc folder is higher priority than example and web.

@nyuichi
Copy link
Member

nyuichi commented Apr 2, 2014

👍

@shibukawa
Copy link
Contributor Author

src/doc/style.css is needed for --doc mode.

@shibukawa
Copy link
Contributor Author

TypeScript doesn't contain any document:

$ tar -tf typescript-1.0.0.tgz 
package/package.json
package/.npmignore
package/CopyrightNotice.txt
package/LICENSE.txt
package/README.txt
package/ThirdPartyNoticeText.txt
package/bin/tsc.js
package/bin/typescript.js
package/bin/lib.d.ts
package/bin/resources/it/diagnosticMessages.generated.json
(omit)
package/bin/resources/zh/tw/diagnosticMessages.generated.json
package/bin/tsc

@shibukawa
Copy link
Contributor Author

I fix a commit a little. jsx --doc works.

@shibukawa
Copy link
Contributor Author

CoffeeScript doesn't have document too:

$ tar -tf coffee-script-1.7.1.tgz 
package/package.json
package/.npmignore
package/README
package/LICENSE
package/register.js
package/CNAME
package/CONTRIBUTING.md
package/bin/cake
package/bin/coffee
package/lib/coffee-script/browser.js
(skip)
package/lib/coffee-script/nodes.js

@shibukawa
Copy link
Contributor Author

Fix pull-request. I moved start codes(src/js) to lib/js-startup.
web folder make archive size 8x (2MB). I should work for that.

@shibukawa
Copy link
Contributor Author

I added bin/jsx-profile-viewer command. It is a webserver only for profiling. Final tgz file size become s 342kB.

@kazuho
Copy link
Member

kazuho commented Apr 7, 2014

Thank you for the PR. Looks promising.

  • how about renaming bin/jsx-profile-viewer to bin/jsx-profile-server? The command not only serves the view but also receives the profile data from client.
  • remove profiler-related code from web/server.js. We would not want to maintain two codes doing the same thing in different files.

@kazuho
Copy link
Member

kazuho commented Apr 7, 2014

PS. I noticed that bin/jsx-profile-viewer binds to port 5000 by default. My assumption is that the profiler should listen at port 2012. Would you mind checking the code to see if it is actually working?

FYI. related documentation is here http://jsx.github.io/doc/profiler.html

@shibukawa
Copy link
Contributor Author

I updated all.

I missed default port is specified in Makefile not server.js (I have noticed default port in server.js (5000) is different from document (2012)). Now it is 2012.

@kazuho
Copy link
Member

kazuho commented Apr 7, 2014

Thank you for the hard work! I think we are almost done.

The last thing I would like to request is to fix profiler crash when accessing the document root (i.e. http://localhost:2012/).

$ bin/jsx-profile-server 
Open http://localhost:2012/

fs.js:166
  binding.stat(pathModule._makeLong(path), cb);
          ^
TypeError: path must be a string
    at Object.fs.exists (fs.js:166:11)
    at serveFile (/mydev/JSX/bin/jsx-profile-server:80:5)
    at Server.<anonymous> (/mydev/JSX/bin/jsx-profile-server:211:3)
    at Server.EventEmitter.emit (events.js:98:17)
    at HTTPParser.parser.onIncoming (http.js:2108:12)
    at HTTPParser.parserOnHeadersComplete [as onHeadersComplete] (http.js:121:23)
    at Socket.socket.ondata (http.js:1966:22)
    at TCP.onread (net.js:527:27)

IMO, the profile page is at /index.html but change all of them to / would be a good idea (e.g. a link to the profiler output is now http://localhost:2012/index.html?2014-04-08-054912 but better be http://localhost:2012/?2014-04-08-054912).

@kazuho
Copy link
Member

kazuho commented Apr 7, 2014

BTW, I have created a separate branch at https://github.com/jsx/JSX/commits/kazuho/experiments/jsx-profile-server. It would be great if you could from now on push changes related to the profile-server stuff to this branch.

The diffs from the changes made in feature/diet-npm-package are:

  • the changes have been split into different commits
    • it is easier to understand if each individual change is stored in different commit
    • for example, I have separated cp web/server.js bin/jsx-profile-server and the changes made to the file into different commits, and it is easier to grasp the changes that were made (see 5577d25)
  • all jquery-related assets have been moved to etc/profiler (instead of copying some assets being used)

PS. I have stripped off the work related to the profile server to PR #313

@kazuho
Copy link
Member

kazuho commented Apr 7, 2014

Oh, and travis has found a problem. https://travis-ci.org/jsx/JSX/builds/22453387

@kazuho
Copy link
Member

kazuho commented Apr 8, 2014

Sorrry for the mess.

To summarize, we need to do the following:

  • fix errors related to bin/jsx-profile-server (split into PR profile server as bin/jsx-profile-server #313)
  • (as discussed on Twitter) add tests that test the npm-packed version of JSX
    • IMO we should add a make target that wraps make test-core with pointing the compiler to the one being packed

@shibukawa
Copy link
Contributor Author

Rebased. This branch can be merged with fast-forward marge again.

@kazuho
Copy link
Member

kazuho commented Apr 14, 2014

@shibukawa

As said, adding necessary tests is mandatory to get this branch merged. Are you working on the issue or want others to help?

  • (as discussed on Twitter) add tests that test the npm-packed version of JSX
    • IMO we should add a make target that wraps make test-core with pointing the compiler to the one being packed

@shibukawa
Copy link
Contributor Author

I need Perl hacker's help > <

@kazuho
Copy link
Member

kazuho commented Apr 14, 2014

Thank you for the response. Will look into it.

@kazuho kazuho closed this in 9e3de9c Apr 14, 2014
@kazuho
Copy link
Member

kazuho commented Apr 15, 2014

Merged to master.

@shibukawa Thank you for your cooperation. I would appreciate it if you would not rebase the code after review since it takes the context of the review away. Splitting commits to smaller chunks, explaining each change as their commit messages would also help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants