-
Notifications
You must be signed in to change notification settings - Fork 1
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 amd loader define to work on linux #2
Conversation
Hey thanks for the PR @luftj! Can you give me a little more info on how you’re using the package? What does your build system look like? Is there a way you could share a small example to reproduce the error? |
it is used inside this project : https://github.com/CityScope/CSL_Hamburg_Grasbrook |
We are working with angular 8. See this minimal example created with the angular CLI (I think you'd need to install it): https://github.com/luftj/maptastictest/tree/master
on the branch https://github.com/luftj/maptastictest/tree/fix I install maptastic not from npm but from my PR-fork, and it works. |
Hey, thank you both for the info/ ways to reproduce - I've been extremely busy at work this week but hope to be able to run this within the week. Thanks for bearing with me, and after this is merged I'll be happy to make either/ both of you maintainers of this repo if you are interested. |
No worries, we've all been there :) And go there regularly...
Sure! I don't have experience, publishing to npm, but we use the maptastic functionality quite regularly. |
I think this is actually that the UMD loader doesn't support ESM modules, which is what Typescript is trying to load the file as. I've changed over to using Rollup to produce the UMD bundle and it seems to fix the issue (I can get a build on your test repo). Still a bit to do there to make it compatible with other loaders, but should be done/ published in the next day or two.
That's ok! I think it's great that you're using this and it's a low stakes package because you're likely the only ones. If you're up for using this a place to learn, you seem like you'd be an excellent maintainer. |
Please let me know if version 2.0.2 solves your problem! I ran it locally against the test repo and everything seemed to be happy. |
Now it compiles but breaks when calling Maptastic()
to reproduce, pull this, npm i, npm start, open localhost:4200 (default) and click on "TEST": https://github.com/luftj/maptastictest/commit/69d686a8c223d29bf6957663cf5fbfa60674cfd9 |
Interesting, I'll see how that's being bundled. Not too familiar with how angular pulls in, but likely need to ship both an esm and umd module. Update: |
Looks like everyone is having similar issues with numeric:
A workaround for you in the short term would be to include numeric in your app HTML: <script src="https://cdnjs.cloudflare.com/ajax/libs/numeric/1.2.6/numeric.js"></script> |
I have dropped the dependency that was having issues and published a new version ( Going to close this now, and I think it has been solved, but would still love to continue to see how you're using this + have you contribute (perhaps adding Typescript defs? #10). |
The new module loader definition is in lower case, as follows:
define(["maptastic"], function() {
This is necessary, because linux (any unix?) uses this as a source path in node_modules/maptastic/dist/... and will fail during
npm start
since the previously used upper case identifier will not be found. Windows node seems to convert cases automatically.I hope this fix won't break anyones import statements, my tests didn't indicate any problems so far.