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

ES2015 imports not working #210

Closed
janusch opened this issue Aug 14, 2018 · 27 comments
Closed

ES2015 imports not working #210

janusch opened this issue Aug 14, 2018 · 27 comments
Labels

Comments

@janusch
Copy link

janusch commented Aug 14, 2018

Hi,
first of all thank you for maintaining this module. I am trying to upgrade from tether-shepard and lots of things have changed.

I have tried both with 2-beta.14 and 2-beta.15 and get two different errors depending on the version.

in 2-beta.14 i get an error TypeError: __WEBPACK_IMPORTED_MODULE_1_shepherd_js___default.a.Tour is not a constructor importing it like this:

import Shepherd from 'shepherd.js';
import Popper from 'popper.js';
window.Popper = Popper;
window.Shepherd = Shepherd;

export function initTour() {
  return new Shepherd.Tour({
    defaults: {
      classes: 'shepherd-theme-default',
      scrollTo: true,
      showCancelLink: true,
    },
  });
}

in 2-beta.15 i get the error: TypeError: Popper is not a constructor

In node_modules/shepherd.js/dist/js/shepherd.js:430 at this.popper = new Popper(opts.element, this.el, popperOpts);

Any idea on what I am missing? Does Shepherd work when it is used within an es6 module?

Some advice would be greatly appreciated!

@RobbieTheWagner
Copy link
Member

@janusch beta 14 was broken. beta 15 should work. It should work without those popper imports. Popper is bundled with Shepherd now, or should be, so just import Shepherd from 'shepherd.js'; should work, as far as I know.

@janusch
Copy link
Author

janusch commented Aug 14, 2018

Hi @rwwagner90,
thank you for your quick reply. I keep getting the same error using 2.0.0-beta-15

TypeError: Popper is not a constructor
Step.setupPopper
node_modules/shepherd.js/dist/js/shepherd.js:430

Also Popper is not defined on window.

Is there anything else I can try?

@RobbieTheWagner
Copy link
Member

@janusch I don't think it should be on the window. It should be bundled with Shepherd. It's possible our UMD output does not work with ES6 imports right now.

@janusch
Copy link
Author

janusch commented Aug 14, 2018

I am using webpack@3 that should not be an issue.
I think I have found the problem it is because I initialize the tour twice. It works fine the first time, and the error comes first when I init the same tour a second time. I'll try to make sure not to init the same tour twice and let you know if that fixes it.

Thank you for your help!

@RobbieTheWagner
Copy link
Member

@janusch you should be able to have multiple tours, so that surprises me.

@janusch
Copy link
Author

janusch commented Aug 14, 2018

@rwwagner90 you were right that isn't the issue.

still getting this:

Uncaught TypeError: Popper is not a constructor
    at Step.setupPopper (shepherd.js:508)
    at Step.render (shepherd.js:719)
    at Step._show (shepherd.js:539)
    at Step.show (shepherd.js:528)
    at Tour.show (shepherd.js:991)
    at Tour.next (shepherd.js:905)
    at Tour.start (shepherd.js:1001)

But first after the tour has already been started. It does show the first step before it blows up.

@janusch
Copy link
Author

janusch commented Aug 14, 2018

Hi again @rwwagner90,
I found the bug though I am not sure on how to fix it yet.
I am apparently importing the amd build line 6 in dist/shepherd.js

When I change this.popper = new Popper(opts.element, this.el, popperOpts);
to: this.popper = new Popper.default(opts.element, this.el, popperOpts); shepherd work fine.

I tried importing shepherd.js from the src directory instead of the dist directory and then I am back to a similar error as I had in beta-14

TypeError: __WEBPACK_IMPORTED_MODULE_1_shepherd_js_src_js_shepherd___default.a.Tour is not a constructor

Any idea on how we can solve this issue?

@RobbieTheWagner
Copy link
Member

@janusch I'm pretty sure the output just doesn't support ES2015 imports. We need to look into it. It's umd format, so you can use CJS, AMD, etc. just can't import right now.

@RobbieTheWagner RobbieTheWagner changed the title Migrations trouble ES2015 imports not working Aug 14, 2018
@janusch
Copy link
Author

janusch commented Aug 14, 2018

@rwwagner90 I think with the latest fixes from 7 days ago make it actually work.

Changes   —   f13736fc ⟷ e71cf03b
-  
+  export default Shepherd; 

That way it is possible to:
import Shepherd from 'shepherd.js/src/js/shepherd';
import Popper from 'popper.js';
window.Popper = Popper;

One will need Popper on window again though.
For me that would be a solution that would work for now.
Would you mind releasing another beta version to npm with the fix that is already in master?

@RobbieTheWagner
Copy link
Member

@janusch I think we want to make popper a peerDependency, and get umd and imports working, so ideally you just:

import Popper from 'popper.js';
import Shepherd from 'shepherd.js';

We have some things in flux right now, so I will release what is in master as the next beta, and hopefully it will help you for now.

@RobbieTheWagner
Copy link
Member

@janusch I published beta 16 earlier. Does it work for you?

@elfenheart
Copy link

I just tried beta 16 and it works for me.

The other caveat is to also do an import of the css theme file that you want:
import "shepherd.js/dist/css/shepherd-theme-dark.css";

@janusch
Copy link
Author

janusch commented Aug 15, 2018

@rwwagner90 Thank you for the beta release. Things are working now.
Ill keep an eye out for changes in shepherd.js and adjust accordingly.

@RobbieTheWagner
Copy link
Member

@elfenheart @janusch I released beta 17, in which I added "module": "src/js/shepherd.js", to package.json. I think that should allow you to just import Shepherd from 'shepherd.js'; if you are using webpack or rollup. I'm definitely not well versed in how to ship a library in both UMD and ES6 modules, so if you could try it out and let me know if I need to make tweaks, that would be great!

@janusch
Copy link
Author

janusch commented Aug 21, 2018

@rwwagner90 Thank you for creating yet another beta. I think it was already working in beta-16. I could import it like this

import Shepherd from 'shepherd.js';
import 'shepherd.js/dist/css/shepherd-theme-default.css';

There is one more issue. When I am building with create-react-app and webpack@3 an optimized production build i get an error:

Failed to minify the code from this file: 
./node_modules/shepherd.js/src/js/evented.js:1 
Read more here: http://bit.ly/2tRViJ9

However shepherd.js works just fine. Maybe I am not using evented.js?

Thank you for your time!

@RobbieTheWagner
Copy link
Member

@janusch the link they give you tells you the issue. Apparently, it does not like ES6. It wants an ES5 build shipped. In that case, you should probably just use the dist version, maybe. We use JS classes, so if your build does not support ES6 and you are not using babel, you should probably use the dist version.

@janusch
Copy link
Author

janusch commented Aug 21, 2018

@rwwagner90 Hey,
I tested a bit again. And in beta-16 all works as intended. beta-17 breaks the webpack create react app build, due to the module field in package.json pointing to the untranspiled src dir. If you add a module field in package.json it would be better to also point to dist/js/shepherd.js. That way it also works, but maybe then the field is redundant all together. I'll stick to the beta-16 for now. We can simply not handle untranspiled code well enough yet.

@RobbieTheWagner
Copy link
Member

@janusch the point of shipping a module, as far as I know, is to ship the vanilla ES6 modules. All others would use the dist code, which I think is what you want here.

@RobbieTheWagner
Copy link
Member

Are you saying import Shepherd from 'shepherd.js' worked for you before? I thought it did not and that was the whole reason for opening this issue?

@janusch
Copy link
Author

janusch commented Aug 21, 2018

yes, from the issue referred above I digged down to that the current create-react-app does use an older version of uglifyjs. Until they upgrade people using create-react-app cannot use libraries that refer in the module field to an ES6 build.

Thank you for the advice. This works perfectly now. Maybe the issue is resolved?

@janusch
Copy link
Author

janusch commented Aug 21, 2018

Are you saying import Shepherd from 'shepherd.js' worked for you before? I thought it did not and that was the whole reason for opening this issue?

it worked since beta-16 which was released after the issue appeared, though the commit fixing it was already in master ;)

@RobbieTheWagner
Copy link
Member

@janusch perhaps we should just remove the module from package.json then, since it worked before using the dist file as main.

@janusch
Copy link
Author

janusch commented Aug 22, 2018

Yes, I think that would be the right thing to do for now.

@anishpateluk
Copy link

anishpateluk commented Dec 13, 2018

This isn't working for me with webpack 4 and shepherd.js beta-18, however, beta-17 is working fine using import Shepherd from 'shepherd.js

@RobbieTheWagner
Copy link
Member

@anishpateluk the latest version is 2.0.0-beta.35. Please use that and let us know if there are issues.

@anishpateluk
Copy link

@rwwagner90 same issue with beta.35

@RobbieTheWagner
Copy link
Member

@anishpateluk in beta 17 we were shipping module, not just main in package.json. I'm not sure if maybe we should ship both again or what to do here.

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

No branches or pull requests

4 participants