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

Form: Uncaught TypeError: this.element.form is not a function #326

Closed
jukkah opened this issue Jul 7, 2016 · 17 comments
Closed

Form: Uncaught TypeError: this.element.form is not a function #326

jukkah opened this issue Jul 7, 2016 · 17 comments
Labels

Comments

@jukkah
Copy link

jukkah commented Jul 7, 2016

Uncaught TypeError: this.element.form is not a function at https://github.com/TechnologyAdvice/stardust/blob/master/src/collections/Form/Form.js#L81.

Variable values at the moment of error:

this.element == $(this.refs.element)
this.element.form === undefined

I'm using stardust with webpack-dev-server and this issue occurs every time I use Form, including

<Form />
@levithomason
Copy link
Member

It looks like jQuery is not loaded. Is your project setup according the usage in the readme? Per the setup instructions, we still require jQuery until we are done removing it.

If you can provide project to reproduce the error after confirming the setup, I can take a look.

@jukkah
Copy link
Author

jukkah commented Jul 7, 2016

It looks like jQuery is not loaded.

No, shouldn't it fail with Uncaught TypeError: $ is not a function if jQuery was not loaded correctly?

Is your project setup according the usage in the readme?

Yep, I think so.

And here is my project: https://github.com/jukkah/short-url

@levithomason
Copy link
Member

Sorry should have been more specific, SUI jQuery is responsible for adding the form function to jQuery. The error indicates SUI JS is not loaded.

I've looked through the project, on my mobile, and it looks correct. I also tested the SUI script src and it loads SUI js. I was able to find "$.fn.form =" in the JS as well, so that is correct.

We use this same setup in the doc site and our production apps, including webpack dev server, so I know it works. I'll have to try this out when I'm back to my computer to further diagnose the issue.

Perhaps @davezuko could run this project and identify any obvious issues for me?

@dvdzkwsk
Copy link
Member

dvdzkwsk commented Jul 7, 2016

I believe that there are two different versions of jQuery being loaded, one from the index.html file, and one from the Stardust dependency (and bundled with webpack). I introspected this by looking at jQuery on the window, which looks to be correct:

screen shot 2016-07-07 at 4 59 44 pm

However, then I took a look at the jQuery module being used within Stardust:

  refresh() {
    this.element = $(this.refs.element)
    window.stardust$ = $

screen shot 2016-07-07 at 5 00 57 pm

I confirmed this by looking at the bundle output and noticed that jQuery is indeed being bundled, so it exists twice, and only one of them is being extended, obviously. A quick and dirty solution is to add this to your webpack configuration:

  externals: {
    jquery: 'jQuery',
  }

This will tell webpack to use the jQuery that you've loaded from the browser. That should get you moving for now, but we really need to tweak the install guide to account for this.

@levithomason
Copy link
Member

Thanks much David. I would call it a bug that we are bundling jQuery with Stardust. I think we should consider updating the build config to use an external jQuery. This will not only make our bundles much smaller, but it will also allow users to choose their own version and build of jQuery.

@dvdzkwsk
Copy link
Member

dvdzkwsk commented Jul 7, 2016

If so, I would add some logic to the root stardust export that throws a huge warning at the user telling them that they need to include jQuery on their own (until we remove it, of course). That way users don't boot up the app without reading through all the instructions and scratch their heads about why jQuery isn't included.

Stardust will have to be updated accordingly, if this is the case, since it relies on local jquery imports, and unless you're going to require people to also bundle semantic jquery extensions (rather than use it from a cdn), then the only sane solution is to access jQuery directly from the window. If you don't, then it becomes important for users to understand in what order the semantic extensions are loaded along with their webpacked bundle, and that's too big of a burden to get up and running.

It's kind of a damned if you do damned if you don't situation, honestly; i.e., requiring jQuery to be on the window makes webpack configuration more complex for first-time users, and the inverse of requiring jQuery to be installed as an npm dependency means users must figure out how to get the semantic extensions loaded correctly.

@levithomason
Copy link
Member

These are all good points. Considering:

  1. jQuery will isn't far from removed
  2. Semantic 2.0 only works with jQuery 2.x
  3. Build updates / complications required to "unbundle" it

Perhaps we instead embrace bundling for a short time. Then we only need to update the README.md. We could technically also do a check to see if jQuery exists on the window, if so, use that one.

@levithomason
Copy link
Member

Actually, I think bundling jQuery with Stardust might make even more sense. Once it is removed, there will be no refactors necessary for users, it is just that the library will no longer include jQuery.

This assumes user's are not likely to be using jQuery with React anyway.

@dvdzkwsk
Copy link
Member

dvdzkwsk commented Jul 7, 2016

@levithomason What about refactoring all import $ from 'jquery' to:

import $ from 'internal/jquery'

// internal/jquery.js
import jQuery from 'jquery'

export default window.$ || jQuery

@levithomason
Copy link
Member

Exactly what I'm thinking, except window.jQuery to be safe.

@dvdzkwsk
Copy link
Member

dvdzkwsk commented Jul 7, 2016

While you're at it, there's probably a test you could do to verify that semantic jquery was loaded, and, if not, throw the user a nice error/warning. This all just being for usability pre-jQuery-removal-enlightenment.

@levithomason
Copy link
Member

Aye. I'm thinking since we also need a specific version of SUI jQuery, we bundle it the same way as jQuery. Including the conditional check to see if it was already loaded. We can check for the SUI plugins we need on jQuery.

@padsbanger
Copy link

Hi guys, I am having the excatly same problem with Progress component, I am getting

Progress.js:83Uncaught TypeError: this.element.progress is not a function

None of the solutions that you have provided works for me ( apart from manually adding jquery and semantic.js via script tag in index.html)

Here is my webpack config:

var path = require('path');
var webpack = require('webpack');

module.exports = {
  devtool: 'source-map',
  entry: [
    'webpack-dev-server/client?http://localhost:1337/',
    'webpack/hot/only-dev-server',
    './src/js/app'
  ],
  output: {
    path: path.join(__dirname, 'dist'),
    filename: 'app.js',
    publicPath: '/dist/'
  },
  module: {
    loaders: [{
      test: /\.js$/,
      loaders: ['react-hot', 'babel'],
      include: path.join(__dirname, './src/js'),
      exclude: /node_modules/
    },
    { test: /\.less$/, exclude: /node_modules/, loader: "style-loader!css-loader!autoprefixer-loader!less-loader"},
    { test: /\.css$/,  loader: 'style-loader!css-loader!autoprefixer-loader'},
    {test: /\.jpe?g$|\.gif$|\.png$|\.ico$/, loader: 'url-loader?limit=100000'},
    {test: /\.eot|\.ttf|\.svg|\.woff2?/, loader: 'url-loader'}
    ]
  },
  plugins: [
      new webpack.optimize.OccurenceOrderPlugin(),
      new webpack.HotModuleReplacementPlugin(),
      new webpack.NoErrorsPlugin(),
      new webpack.DefinePlugin({
          'process.env': {
              NODE_ENV: JSON.stringify('development'),
              APP_ENV: JSON.stringify('browser')
          }
      })
  ],
  externals: {
    jquery: 'jQuery',
  }
};

Am i shimming/exposing jQuery good ? Any other possible fixes for that ?

@levithomason
Copy link
Member

@padsbanger I'm currently working on this against the example repo provided by @jukkah above. Hope to have a fix up here soon.

@levithomason
Copy link
Member

levithomason commented Jul 10, 2016

@jukkah @padsbanger I've released 0.19.0 which solves the jQuery issues. I submitted jukkah/short-url#1 to @jukkah's project so you can see what a working project should look like.

In short, do not include jQuery nor Semantic UI JS on the page. Also, do not include any config for these in your Webpack config. It is all handled by Stardust now.

If you do have jQuery on the page, you'll notice Stardust will tell you to remove it:

image

If you turn on debugging for jQuery localStorage.debug = 'stardust:jquery' you can see when and what Stardust is loading:

image

Let me know if you have any other issues.

@padsbanger
Copy link

Thank you Sir, it works perferctly !

@levithomason
Copy link
Member

Awesome! Very glad for that 😸 I'm working on the jQuery free progress right now. Should have that done toady as well.

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