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.

Sign up for GitHub

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

Jump to bottom

Shift to Webpack based build system for JS files #224

Merged
merged 14 commits into from
Sep 19, 2019
Merged

Shift to Webpack based build system for JS files #224

merged 14 commits into from
Sep 19, 2019

Conversation

apsdehal
Copy link
Contributor

@apsdehal apsdehal commented Sep 18, 2019 •
edited
Loading

Motivation and Context

This PR introduces Webpack based build system which will allow us to target older browsers while using newer > ES6 features. Following features are introduced:

  • Webpack based compilation which
    • Bundles everything into single file
    • Also checks for ESLint errors
    • Also generators HTML file from bindings.html with version numbers to bust cache
    • CSS can be directly imported into JS files now and will be attached inline to the final generated file
  • We use babel to transpile everything
  • Update CMakeLists as well to use webpack
  • Builds source maps as well
  • Move all files to module based ES6 system

In future, I will look into compile hsim_bindings.js also into the same file.

How Has This Been Tested

./build_js.sh and bindings.html in browser.

For Building On Your Side

Pull the change locally, run npm install, run ./build_js.sh and open bindings.html in browser.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have completed my CLA (see CONTRIBUTING)
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Sep 18, 2019
@msbaines
Copy link
Contributor

I get an exception when I try this:

___cxa_throw @ hsim_bindings.js:1
  (anonymous) @
  (anonymous) @
  (anonymous) @
  (anonymous) @
  (anonymous) @
  Module.dynCall_iii @
  dynCall_iii_162 @
  constructor_body @
  (anonymous) @
  Simulator @
  t @
  Module.onRuntimeInitialized @
  doRun @
  (anonymous) @
  setTimeout (async)  
  run @
  runCaller @
  removeRunDependency @
  receiveInstance @
  receiveInstantiatedSource @
  Promise.then (async)  
  instantiateArrayBuffer @
  (anonymous) @
  Promise.then (async)  
  (anonymous) @
  Promise.then (async)  
  instantiateAsync @
  createWasm @
  Module.asm @
  (anonymous) @

@msbaines
Copy link
Contributor

Is it possible to make this optional? It makes debugging difficult.

msbaines
msbaines reviewed Sep 18, 2019
View reviewed changes
src/esp/bindings_js/webpack.config.js Outdated
const HtmlWebpackPlugin = require("html-webpack-plugin");

// All of the files will be outputted in build_js
const buildRootPath = path.resolve(__dirname).replace("src", "build_js");
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to avoid the use of build_js here. Maybe using cmake's configure_file?

msavva
msavva reviewed Sep 19, 2019
View reviewed changes
Copy link
Collaborator

@msavva msavva left a comment

Choose a reason for hiding this comment

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

I don't have the context to do a detailed review, but overall moving to a js build system that supports modules and incremental builds is a great idea!

mosra
mosra reviewed Sep 19, 2019
View reviewed changes
Copy link
Collaborator

@mosra mosra left a comment

Choose a reason for hiding this comment

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

This goes wayy over my head, I have no idea how most of those things work.

But formatting looks okay 😅

@msbaines
Copy link
Contributor

Is it possible to make this optional? It makes debugging difficult.

Amanpreet showed me where I can find the sources in the chrome debugger so this is no longer a concern.

msbaines
msbaines reviewed Sep 19, 2019
View reviewed changes
src/esp/bindings_js/bindings.html Outdated
</body>

</html>
</html>
Copy link
Contributor

Choose a reason for hiding this comment

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

Add newline.

@apsdehal apsdehal merged commit be178ed into facebookresearch:master Sep 19, 2019
@apsdehal apsdehal deleted the webpack-build branch September 19, 2019 23:18
eundersander pushed a commit to eundersander/habitat-sim that referenced this pull request Aug 6, 2020
@mathfac
Updated class diagram, added link to the demo, added stable version c…
61e9eed 
…heckout (facebookresearch#224)
Ram81 pushed a commit to Ram81/habitat-web-sim that referenced this pull request Dec 10, 2020
@apsdehal
Shift to Webpack based build system for JS files (facebookresearch#224)
17e2e88 
* Add new deps for webpack based building
* Add webpack configuration
* Shift to module based system for ES6 loading
* Update CMakeLists
* Remove unsused vars checks from navigate and simenv
* Update cmakelists to do npm install as well
* Update circle ci also to do npm install
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mosra mosra mosra left review comments

@msavva msavva msavva left review comments

@msbaines msbaines msbaines approved these changes

@abhiskk abhiskk Awaiting requested review from abhiskk

@erikwijmans erikwijmans Awaiting requested review from erikwijmans

Assignees
No one assigned
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@apsdehal @msbaines @mosra @msavva @facebook-github-bot