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

130 unpack main #29

Open
1 task
JonathonDunford opened this issue Mar 27, 2018 · 8 comments
Open
1 task

130 unpack main #29

JonathonDunford opened this issue Mar 27, 2018 · 8 comments
Assignees

Comments

@JonathonDunford
Copy link
Member

JonathonDunford commented Mar 27, 2018

Issue by activescott
Sunday Mar 18, 2018 at 10:58 GMT
Originally opened as https://github.com/forkdelta/forkdelta.github.io/pull/131


NOTE: I suggest we ignore the content below here and use the PR for the latest information at #35

This definitely needs a lot of testing but appears to be working.

To test just checkout the branch and run as usual.

To re-build js/main-bundle.js from the modules run npm run build from the root directory.

To unpack the modules from the old minified code again

  1. Extract the huge array of modules embedded inside of js/main-unminified.js and put it in scripts/_old_main_packed_modules.js note: If you look carefully this whole ~150K line array is an argument to a function, so I can't think of a clean way to grab it from the script without more sophisticated (and error pone) parsing. Alternatively, if this branch is going to live for more than a week or so we could just adjust js/main-unminified.js to pop that array out into a field in the file and then I could safely get at it from the script. Since this is a one-time thing though I think it is just as easy to just manually extract it.
  2. From scripts, run node unpack_modules.js (it unpacks from scripts/_old_main_packed_modules.js)
  3. In the generated scripts/unpacked_modules/main.js there is one require('../server/utility.js') that needs converted to require('./server/utility.js')
  4. cp -r scripts/unpacked_modules/* js/
  5. Then re-build js/main-bundle.js by running npm run build from the root directory

This is a one-time thing so we shouldn't need the script or to re-run this unless we need it for some merges before this gets merged.

  • We can test this as is for now, but before merging we need to regenerate the modules using the script (as described above) or manually pull in commits after this branch was pulled from master like this one as due to the abandonment of in main-unminified.js in this branch git won't be able to auto merge.

activescott included the following code: https://github.com/forkdelta/forkdelta.github.io/pull/131/commits

@JonathonDunford
Copy link
Member Author

Comment by Ebonsignori
Sunday Mar 18, 2018 at 19:43 GMT


Amazing. How was this accomplished? Did you write scripts/unpack_modules.js yourself? I was looking into unpackjs earlier, but without any luck.

Edit: Checked out and everything seemed to be working smoothly.

@JonathonDunford
Copy link
Member Author

Comment by activescott
Sunday Mar 18, 2018 at 23:17 GMT


@Ebonsignori I wrote scripts/unpack_modules.js last night. It's not pretty code, but it works. Maybe once we test this and get it working on forkdelta I'll clean that up and put that in another repo and we can test with the various bundlers out there and see just how robust we can get it.

If you can test this branch a bit and confirm things work that would be much appreciated!

@JonathonDunford
Copy link
Member Author

Comment by JonathonDunford
Tuesday Mar 20, 2018 at 03:39 GMT


@activescott

So, the only thing that needs to be done is to run the script and change that one require line?

My initial concern is we wouldn't be able to see all changes made, but we could verify that the script to unpack works as intended and then run it ourselves.

I tested it and it works.

Awesome work.

@JonathonDunford
Copy link
Member Author

Comment by activescott
Tuesday Mar 20, 2018 at 04:23 GMT


@JonathonDunford Almost. There is one step to manually extract the array of modules from js/main-unminified.js before running the script (I updated the instructions above). Then just run the script and it will unpack the modules. The one edit to the require is just easier than enhancing the script to support that weird directory structure.

As long as a couple of us do a solid test pass and it works I think we should merge it. I suggest we get any other PRs that impact the main-unminified.js in first (only #125 AFAIK), then re-run the scripts to get the latest changes. We do a final quick test pass and then merge it. Once we do the merge though all bugs/enhancements would of course be in the new unpacked code 🎉 .

I don't really expect any problems (Famous last words I know). I expect either everything will work or almost nothing would or there would be something glaringly wrong. If something were wrong, my guess is a whole module got missed, and it would be easy to track it down and unpack it manually in the worst case.

@JonathonDunford
Copy link
Member Author

Comment by activescott
Sunday Mar 25, 2018 at 23:06 GMT


@forkdelta/frontend-team Would be great if you can all download this branch and run through some tests. Would really love some testing on the most critical operations like buy, sell, transfer, withdrawal, deposit, etc.

On a related note, do we have a list of test cases anywhere?

@activescott
Copy link
Contributor

BTW: Let me send a new PR for this that I'm working on in branch at https://github.com/activescott/FrontEnd/tree/unpack_main

@activescott activescott self-assigned this Apr 12, 2018
@JonathonDunford
Copy link
Member Author

@activescott What's the status on this?

I don't see a PR in frontend

@activescott
Copy link
Contributor

PR is at #35

@activescott activescott mentioned this issue May 3, 2018
5 tasks
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

No branches or pull requests

2 participants