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

Fix webpack build error 'dependency to an entry point is not allowed' #1447

Merged
merged 2 commits into from May 31, 2016

Conversation

Projects
None yet
3 participants
@elhigu
Collaborator

elhigu commented May 25, 2016

No description provided.

export default function makeKnex(client) {
export default function makeKnex(client, version) {

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 26, 2016

Collaborator

Hmmm... Are we sure this will work correctly? I'm quite certain makeKnex is called from more places than just src/index.js and those places would then need to be updated as well or they'll set the created knex instance's VERSION attribute to undefined.

I'm not quite sure - why is it that we want to add this version parameter anyway? Can we instead just move the whole version code into a separate module and then include it from there to both here and the code that sets it on the Knex class?

This comment has been minimized.

@rhys-vdw

rhys-vdw May 26, 2016

Collaborator

Simplest solution: Just require package.json in both locations.

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 26, 2016

Collaborator

Agreed.

And it was actually my suggestion that we read the version from the class here that broke this. 😭

Though if it were up to me, I'd add a version.js file as an interface layer, have it import the package.json file and export just the version information from there, and then require that version.js file from wherever the version is needed. But that's just because importing a .json as anything by a localized implementation detail smells bad to me. 👿

This comment has been minimized.

@elhigu

elhigu May 26, 2016

Collaborator

Yep... actually we already have that version in external package.json file, which was my second option for implementing this :) I'll change this one to read package json directly.

EDIT: I went with importing package.json twice... if we need to extract more data from there, maybe we could write a wrapper class or something for it. Currently it would be just unnecessary middle phase when getting version...

@elhigu

This comment has been minimized.

Collaborator

elhigu commented May 26, 2016

Still need to add build success test for webpack...

@elhigu

This comment has been minimized.

Collaborator

elhigu commented May 29, 2016

@jurko-gospodnetic I added running webpack build during npm test. I had to add one webpack plugin there to make webpack to return exit code 1 when there are errors during execution.

@elhigu elhigu added the bug label May 29, 2016

@rhys-vdw rhys-vdw merged commit e657aae into tgriesser:master May 31, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment