Skip to content

Use dist/index.js as the package's entry point #178

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

Merged
merged 1 commit into from
Mar 21, 2017
Merged

Use dist/index.js as the package's entry point #178

merged 1 commit into from
Mar 21, 2017

Conversation

adi-pjackson
Copy link
Contributor

Potential fix for #177

@jsf-clabot
Copy link

jsf-clabot commented Mar 20, 2017

CLA assistant check
All committers have signed the CLA.

@hckhanh
Copy link

hckhanh commented Mar 21, 2017

I get this bug when I try to upgrade to v4 as well. Hope this bug will be fixed soon.

@adi-pjackson
Copy link
Contributor Author

adi-pjackson commented Mar 21, 2017

The appveyor failure looks like a permission issue, so hopefully a false negative 🤞

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Mar 21, 2017

Closes #161 #179 #177

@adi-pjackson Thx

cc @jhnns Sry for making stress here 😛 but this one is important and should be temporarily fix the issue

@jhnns
Copy link
Member

jhnns commented Mar 21, 2017

Damn it! 😁 Thx for the PR 👍

@michael-ciniawsky Is this a bug in webpack-defaults? Or should we use ES2015 modules everywhere?

@jhnns jhnns merged commit 918bfe9 into webpack-contrib:master Mar 21, 2017
@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Mar 21, 2017

@jhnns Yep, seems it works with 0.4.4 now, ES2015 Modules weren't transpiled to CJS and the src/cjs.js wrapper wasn't added automatically, but changed in package.json 😛

I tried updating to import/export all over the code base, but resolver related tests failed and we should in general test the transpiled code in dist to be sure there are no bugs in the build step, which likely needs changes to webpack-defaults again, better keep it as is for the moment :D

Maybe even just stick to require()/module.exports would be the saner path in general :)

@jhnns
Copy link
Member

jhnns commented Mar 21, 2017

I tried updating to import/export all over the code base, but resolver related tests failed and we should in general test the transpiled code in dist to be sure there are no bugs in the build step,

Yes. To be honest, I'm not very comfortable to build something and immediately release it without testing it. I'm paranoid in this regard ^^

Maybe even just stick to require()/module.exports would be the saner path in general :)

Yes, I think so. Makes some things easier and CommonJS is just fine for Node.js.

@michael-ciniawsky
Copy link
Member

Yes. To be honest, I'm not very comfortable to build something and immediately release it without testing it. I'm paranoid in this regard ^^

Yep, not a bad habit when it comes to code 😛 . Should I nevertheless send PR (can be closed) or better omit unnessecary noise and you take a look later own your own?

@jhnns
Copy link
Member

jhnns commented Mar 21, 2017

Should I nevertheless send PR (can be closed) or better omit unnessecary noise and you take a look later own your own?

What do you mean? ^^ The issue is fixed now...

@adi-pjackson adi-pjackson deleted the fix-package-main branch March 21, 2017 21:50
@michael-ciniawsky
Copy link
Member

Temporarily fixed 😉 You can not use webpack-defaults if the cjs.js wrapper isn't added or it is removed from wepack-defaults in the future

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.

5 participants