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

Better webpack support #164

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

briancappello
Copy link
Contributor

Changes:

  • I modified src/techan.js to be more flexible about where/how it expects d3 to have been made available
  • Grunt now generates the version file inside the src/ tree, and this version file is included in the repository. This allows webpack to work with both the included dist/techan.js and the un-optimized src/techan.js (otherwise, it is necessary to fork the repository, run npm install, run npm link, and finally run grunt, all just to generate the one version file)
  • I added a usage note to the README specifying details for how to import the library in ES6 code.

resolve: {
...,
alias: {
'techan': path.resolve('node_modules/techan/src/techan.js')
Copy link
Contributor Author

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.

@briancappello
Copy link
Contributor Author

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';
Copy link
Contributor Author

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?

Copy link
Contributor Author

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.

@andredumas
Copy link
Owner

andredumas commented Oct 10, 2016

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 src/techan and given the way it bootstraps itself (immediately initialising full structure and packages), I'm interested to hear from your use if webpack code splitting/minification works as expected and this indeed is the only required change.

I've also never been happy committing the built dist, seems like a clumsy after step, comitting version.js seems to add to this yet I can't think of a nicer way around it. Having version details built in is definitely required.

@briancappello
Copy link
Contributor Author

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 :/

@andredumas
Copy link
Owner

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.

@thomashan
Copy link
Contributor

The move to ES6 sounds good.
Maybe you can setup a ES6 branch for us to work on so that we can start to refactor things.

@andredumas
Copy link
Owner

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

@briancappello
Copy link
Contributor Author

@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!

@adamrabie
Copy link

adamrabie commented Nov 21, 2016

I'm having issues here too, getting error on webpack:

Critical dependencies:
5:479-486 This seems to be a pre-built javascript file. Though this is possible, it's not recommended. Try to require the original source to get better results.
 @ ./~/techan/dist/techan.js 5:479-486

and in the browser:

image

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.

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

Successfully merging this pull request may close these issues.

4 participants