-
Notifications
You must be signed in to change notification settings - Fork 64
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
Inital commit demo site #16
Conversation
Pull Request Test Coverage Report for Build 130
💛 - Coveralls |
Pull Request Test Coverage Report for Build 132
💛 - Coveralls |
Good morning @rowanwins - thanks much for the contribution! Appreciate you setting this up. I would like to, as much as possible, contain stuff for the demo site in the I will do a more complete pass over this within the week. Again, thanks for setting it up. |
Hey @mfogel i think that should mostly be doable, I'll take a look tonight and report back |
Hi @mfogel Everything is now in the docs folder, except the following config files - webpack config file, babelrc file, and an eslintrc file. I just picked a few random test cases to show but you could show any which you think a good. I've also included some performance stats against other libs in there but that can be easily dropped out at some stage. If you'd like to add actual API docs (eg more than what's in the README.md) at some stage let me know. |
src/sweep-line.js
Outdated
@@ -1,4 +1,4 @@ | |||
const SplayTree = require('splaytree') | |||
import SplayTree from 'splaytree' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kinda confused by this change... assuming it's intentional - do you know why splaytree needs to be imported like this and the other modules are ok with the require()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I need to look into that further, it's probably a webpack config thing which I should be able to fix. ES6 modules and bundling tools are still a bit complicated sometimes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what's going on is that webpack is loading the es module for splaytree
because it sees the module
definition in splaytree's package.json
This is conflicting with the require('splaytree')
call - which expects a common JS module.
I don't think this is the 'right' solution, but another way to avoid this conflict for the purposes of this PR is to make a change to the webpack.config.js
to force the splaytree import to load the common JS version of splaytree rather than the es6 module. This is one way to get webpack to do that (maybe not the best way?):
--- a/webpack.config.js
+++ b/webpack.config.js
@@ -42,6 +42,7 @@ module.exports = {
},
resolve: {
alias: {
+ 'splaytree$': 'splaytree/dist/splay.js',
'vue$': 'vue/dist/vue.esm.js'
},
extensions: ['*', '.js', '.vue', '.json']
Applying this change to webpack.config.js
then makes this change from require()
to import
in src/sweep-line.js
unnecessary, which I like because then these source files can all still be loaded directly by node with no intermediate compile/build step. Down the road, I would like to add npm run build
step to build both common JS and es modules, but I think better to address that separately and not mixed in with this PR.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try and take a look into this tonight, been away for the weekend sorry
package.json
Outdated
}, | ||
"dependencies": { | ||
"qheap": "^1.4.0", | ||
"save-dev": "^2.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems this package isn't a real package... unintentional add?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad!
Have updating that splaytree handling @mfogel - sorry for the delay :) |
@rowanwins thanks for making those changes, I merged this all in in a squash commit. It's now available at http://mike.fogel.ca/polygon-clipping/ - but stay tuned I will get this set up at a js.org subdomain. |
Now live at https://polygon-clipping.js.org/ |
Gday @mfogel
I've finally had a bit of time to create a demo site.
A bit of explanation
spread operator
andarrow syntax
you've used isn't also cross-browser friendly)npm install
, this will install a few more dev dependenciesnpm run dev
, this'll start a site atlocalhost:8080
npm run build:docs
https:// mfogel.github.io/polygon-clipping/docs
Hopefully if you're a little bit familiar with Vue or React it'll make sense, although if you're not terribly familiar with modern front-end dev it might all seem a bit weird!
Let me know what you think.