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

Fixed import bug #116

Merged
merged 1 commit into from
Apr 10, 2019
Merged

Fixed import bug #116

merged 1 commit into from
Apr 10, 2019

Conversation

aviklai
Copy link
Contributor

@aviklai aviklai commented Apr 7, 2019

Fix for #115.

@MatthewHerbst
Copy link
Owner

Thanks for the PR! Can you explain why this fixes it please? Changes make sense, just not sure what the issue with the current setup is

@aviklai
Copy link
Contributor Author

aviklai commented Apr 7, 2019

@MatthewHerbst Hi, unfortunately I am not sure either why the previous setup didn't work but it looked like an issue with the entry point. I stumbled upon the issue after having the exact same error on my project.

I checked this new setup on a project I am working on and it looks o.k.
You can try to download from my fork and check that it works.

@hendra-go
Copy link

hendra-go commented Apr 9, 2019

I can confirmed that this PR works.
But i think like @MatthewHerbst said, this is an export issue, not import issue, isn't it?

And i think is because UMD (Universal Module Definition) is well... more "universal" and compatible with both AMD and CommonJS

@MatthewHerbst
Copy link
Owner

And i think is because UMD (Universal Module Definition) is well... more "universal" and compatible with both AMD and CommonJS

Was the previous build (before 2.1) UMD? @sergeyshmakov did the TS PR change the build type?

@MatthewHerbst
Copy link
Owner

@gregnb do you have any knowledge here? I'm thinking we merge this unless anyone thinks it will cause other problems. @cmckni3 do you see any issues with the existing TS config, or with this PR?

@cmckni3
Copy link
Contributor

cmckni3 commented Apr 10, 2019

Ah yes. The TS conversion added libraryTarget: "commonjs" to the webpack. UMD should be published to npm so it can be consumed in multiple environments (commonjs, AMD, ESM, etc).

@MatthewHerbst MatthewHerbst merged commit 5b17a82 into MatthewHerbst:master Apr 10, 2019
@MatthewHerbst
Copy link
Owner

@gregnb can you please publish this fix as 2.1.1? thanks!

Thanks @cmckni3!

@gregnb
Copy link
Collaborator

gregnb commented Apr 11, 2019

Soon as I land from vacation Saturday. Sorry for the delay

@gregnb
Copy link
Collaborator

gregnb commented Apr 13, 2019

@MatthewHerbst I'm still swamped from traveling. I've added you to have publishing access on npm. Can you test let me know if you have any issues?

@MatthewHerbst
Copy link
Owner

v2.1.1 is published. Please report any problems. Thank you everyone for the help fixing this!

(Thanks for the privileges @gregnb!)

@gregnb
Copy link
Collaborator

gregnb commented Apr 13, 2019

Awesome! No problem thanks for all your help :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants