-
Notifications
You must be signed in to change notification settings - Fork 534
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
Better webpack support #164
base: master
Are you sure you want to change the base?
Better webpack support #164
Conversation
resolve: { | ||
..., | ||
alias: { | ||
'techan': path.resolve('node_modules/techan/src/techan.js') |
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.
This is not strictly necessary, but it allows webpack to work with the un-optimized source code instead of the dist/techan.js
specified in package.json
's main key. This way, end user's webpack configs can do whatever minification / code splitting / etc the deem necessary, instead of relying on the assumptions ingrained in the dist file. It also allows for getting accurate source maps should somebody prefer to do development on techan from their webpack-based project instead of the recommended approach.
I'll be the first to admit that I'm not super-familiar with javascript library packaging conventions, but, out of the box this library is difficult to use with a webpack-based project, and this is my attempt at improving the experience. My primary concern with this PR would be if I broke any existing methods of working with the library (hopefully not!) |
} | ||
|
||
// usage | ||
import * as d3 from 'd3'; |
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.
Okay, so this doesn't work as expected with anything requiring mouse events (eg crosshairs). d3/d3#2733
I'm still searching for a solution that lets techan.js
's events work with babel. Do you perhaps have any insights?
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 just removed my README modifications all-together. From my research, it seems like the problem i was experiencing is a bug in babel and unrelated to your library. The workaround was to simply include d3 with a typical script tag, outside the flow of babel.
3e6f946
to
69a32ef
Compare
Thanks for the contribution @briancappello! You've been crazy busy, thanks. This may take me a bit to review as I'm not too familiar with webpack's workings. It's been on my todo list for a while so maybe the time is now 😄 techan was never designed to be sourced directly from I've also never been happy committing the built |
It's my pleasure, thanks for all of your hard work on the library! RE: webpack, as long as the version file exists, and the alias to src/techan.js is set, as best I can tell everything works as expected. Since I'm using babel to transpile ES6 into ES5, I had to manually include d3 (because of that d3 issue I linked which I don't really understand). Webpack is kind of a beast, but its documentation is slowly improving. Once you can wrap your head around it, it's plugin-based and declarative, which makes it easy to conditionally (dev vs prod) add hot reloading, minification, sass to css, etc. I got into it for building React web apps, so I don't really have any experience with using it as a replacement for gulp/grunt on individual library projects - my impression though is that it's more targeted at bundling up applications. But I'm also rather new to the world of web development (started with ES6...), so you know, my 2 cents probably aren't even worth that :/ |
I'm still reviewing this, but you have made me seriously think of an overhaul of the code base, mainly to cut over to ES6 modules, package namespace flatten and general housekeeping. I set the project up a couple of years ago, and there are much better tooling and approaches now. A lot of things I added then thinking it might be useful is adding overhead and increasing barrier to entry and will play much nicer with tools such as webpack or rollup. |
The move to ES6 sounds good. |
Hey @thomashan Good call, I think I will. I'll poc out a few options and will push a way that I think is good for review. andre |
@andredumas Just following up here, were you able to make any progress on your branch? I was also wondering, do you have a roadmap of high-priority TODOs/goals to achieve before a 1.0 release was to be declared? (Before/after any kind of big refactor / ES6 switchover?) Would love to know a bit more about your thoughts with regard to the direction(s) you had in mind! |
I'm having issues here too, getting error on webpack:
and in the browser: Maybe this should be a separate issue but to resolve it I ended up including techan dist file in my project, referencing in the html src and updating eslintrc to add techan to global space. |
Changes: