Skip to content

ES6, webpack, Babel, libraries imported through npm, and NodeJS version #95

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

Merged
merged 20 commits into from
Mar 30, 2017

Conversation

n1474335
Copy link
Member

@n1474335 n1474335 commented Mar 27, 2017

Summary

This pull request represents a substantial restructuring of the project. There are no visible changes to the web app itself, however a number of changes have been made to the build process and the repository structure.

Main changes

  • ES6 module imports/exports have been introduced.
  • Babel is now part of the build process, allowing the use of ES6 syntax throughout the project. Babel has been configured to target the following browser versions, making CyberChef more backwards compatible whilst supporting the latest JavaScript features:
    • Chrome 40
    • Firefox 35
    • Edge 14
  • Webpack is now used to build the app and import libraries through npm, instead of including them statically as before. One or two libraries are still included statically, however work should be put into replacing these or adding them to npm in the future.
    • It was decided that Grunt would be kept as it still provides various useful features that are not as elegant when reproduced with npm run scripts.
  • A NodeJS version of CyberChef can now be built using grunt node and then imported and run as follows:
> var chef = require("./CyberChef.js")
undefined
> chef.bake("test", [{"op":"To Hex","args":["Space"]}])
{ result: '74 65 73 74',
  type: 'string',
  progress: 1,
  options: {},
  duration: 2,
  error: false }
  • The source code has been restructured into three main sections:
    • src/core containing all the code which handles the actual baking process, including all the operations
      • Code within src/core should now aim to be cross-platform (i.e. it should work in NodeJS and the browser). Webpack handles a lot of this for you, however there will be some cases where care needs to be taken.
    • src/web containing the code required to run the web app
    • src/node containing the code required to run the NodeJS version
  • Now that operations can be run in Node, the test runner has been simplified and the dependency on a headless browser removed.

Issues this addresses

This pull request should address the following issues once merged:

#2 Use a modern library management system
#5 use babel and babel-preset-env

It will also become much easier to fix compatibility issues such as #39 (Drag item to Favourites on iPad does not work).

Known bugs

This number of changes will inevitably have introduced some bugs. If you have the time, please contribute tests and (ideally) bug fixes. I will leave this unmerged for a little while to give us a chance to go through it. If you have any comments or code reviews, I would be glad to hear them.

  • The test runner no longer times out if a test is taking too long - this should probably be reintroduced
  • The before and after compression stats in src/web/static/stats.txt are not correctly populated when running a production build
  • There are still various calls to window and document in src/core which will break the NodeJS version
  • The 'Diff' operation appears to be broken
  • 'Entropy' and 'Frequency distribution' operations are broken as the CanvasComponents lib is not defined

@tlwr tlwr mentioned this pull request Mar 28, 2017
@n1474335
Copy link
Member Author

As far as I'm aware, this branch is now ready to merge. I'll leave it open for one more day in case anyone else would like to review it.

@n1474335 n1474335 merged commit 8ed4cb6 into master Mar 30, 2017
@n1474335 n1474335 deleted the feature-webpack branch March 30, 2017 17:25
@graingert
Copy link

@n1474335 do you want https://www.npmjs.com/package/cyberchef on npm?

@@ -26,21 +26,55 @@
"url": "https://github.com/gchq/CyberChef/"

Choose a reason for hiding this comment

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

it's probably worth adding a prePublish hook for compiling the node bundle

node: {
target: "node",
entry: "./src/node/index.js",
output: {

Choose a reason for hiding this comment

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

Should use https://www.npmjs.com/package/webpack-node-externals so that the node build doesn't contain all of the 3rd party deps

@n1474335
Copy link
Member Author

@graingert, yes if you could transfer the npm package over to me (https://www.npmjs.com/~n1474335), that would be great.

Thanks for your comments about improving node integration and more generally all the work you did on ES6 and webpack - it was a very useful reference.

@graingert
Copy link

@n1474335 added

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

Successfully merging this pull request may close these issues.

2 participants