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

jQuery 3.4 errors with src directory webpack alias #4358

Closed
almercier opened this issue Apr 16, 2019 · 16 comments

Comments

Projects
None yet
5 participants
@almercier
Copy link

commented Apr 16, 2019

We've received errors going from 3.3.1 to 3.4.0 with loading jquery through webpack.
I'm noticing that the errors no longer appear after removing the alias we are using in webpack for jquery per this user's SO suggestion #1

resolve: {
    alias: {
        // Alias `import $ from 'jquery'` to resolve to the src JS file
        jquery: 'jquery/src/jquery'
    },
},

We are loading and using jquery per webpack's suggestion here

// Load jquery
new webpack.ProvidePlugin({
    $: 'jquery',
    jQuery: 'jquery',
}),

The error for me is happening when we call jquery-ui "draggable" on a jquery wrapped element.
Cannot read property 'position' of undefined. finalPropName.js:26 which is on the following line var final = jQuery.cssProps[ name ] || vendorProps[ name ];. Upon debugging on that line immediately before the error is thrown, I am seeing that the variable jQuery is just an empty object while window.jQuery is the actual jQuery instance...

Is this a bug and/or undocumented breaking change in the src/jquery.js file of the new version?

@timmywil

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

Thanks for opening an issue, but there were no changes to the src/jquery.js file itself or the way it is loaded with AMD. You can see exactly what we distribute at this repo. This issue is pretty vague. If you're able to narrow it down and provide a small test case from https://jsbin.com that reproduces the error you're seeing in combination with draggable, feel free to post it here and we can reopen.

@timmywil timmywil closed this Apr 16, 2019

@dmethvin

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

I think the error is caused by finalPropName.js not referencing jQuery via "../core" in the AMD function definition. So, it ends up using the global jQuery. We probably don't have a test that would cause this to fail.

@dmethvin dmethvin reopened this Apr 17, 2019

@timmywil

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

Interesting. I ran tests in AMD mode and didn't see this, but you're right.

@timmywil timmywil added this to the 3.4.1 milestone Apr 17, 2019

@dmethvin

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

Yeah not sure why, we must have somehow polluted the global space with a jQuery in AMD mode.

@dmethvin dmethvin self-assigned this Apr 17, 2019

@almercier

This comment has been minimized.

Copy link
Author

commented Apr 17, 2019

Would it be helpful if I made a super simple public repo with instructions that reproduce? It doesn't seem like it would be possible to repro on something like jsbin.

@almercier

This comment has been minimized.

Copy link
Author

commented Apr 17, 2019

So, it ends up using the global jQuery

This would make sense as to why it might be erroring for me, as loading jquery through webpack's providePlugin does not inject jquery to the global namespace.

mgol added a commit to mgol/jquery that referenced this issue Apr 17, 2019

Build: Fix unresolved jQuery reference in finalPropName
Also, prevent further similar breakages by changing our ESLint configuration
to disallow relying on a global jQuery object in AMD modules.

Fixes jquerygh-4358
@mgol

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

PR: #4361

@almercier The biggest help right now would be if you applied the code from PR #4361 in your node_modules locally and checked if it resolves your issue. If that's the case, the fix will be included in jQuery 3.4.1.

@dmethvin Sorry, I didn't notice your assignment to this issue & I submitted a PR myself. We can still investigate why tests worked in AMD mode at all, i.e. how come the global jQuery was available.

@almercier

This comment has been minimized.

Copy link
Author

commented Apr 17, 2019

@mgol swapping with #4361 resolved my issue 👍

@mgol

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

@dmethvin

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

No problem @mgol, any time I can get someone to clean up my mess I'm happy! I will look into why there's a global jQuery, The global jQuery is coming from jQuery 1.9.1 in index.html which we should rename to something else I think.

@dmethvin

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

Oh right, we needed a "jQuery not under test". So I think just renaming the test jQuery might work. We'll also need to use a somewhat fresher one once we get to 4.0 .

@timmywil

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

Yea, I thought we already renamed to supportjQuery, but I guess that's just a local var.

@lucassonsinlima

This comment has been minimized.

Copy link

commented Apr 17, 2019

Hi! I'm facing the same issue, you guys have any idea when you'll release the update?

@timmywil

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

I'd say sometime next week, depending on the team's availability.

@mgol

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

@timmywil @dmethvin I'm not sure it's supportjQuery. In AMD mode jQuery.fn.jquery in the console evaluates to me to "@VERSION", not 1.9.1. Also, the way supportjQuery is defined should unset the jQuery global:

const supportjQuery = this.jQuery;

supportjQuery.noConflict( true );
window.originaljQuery = this.jQuery = undefined;
window.original$ = this.$ = "replaced";

So it's our current global that gets exposed.

See:
https://github.com/jquery/jquery/blob/3.4.0/src/exports/global.js#L27-L32
The global is exposed in AMD mode, it's only hidden in CommonJS and only if there's a global document variable, otherwise we expose it as well (that might actually be a bug?), compare:
https://github.com/jquery/jquery/blob/3.4.0/src/wrapper.js#L29
with:
https://github.com/jquery/jquery/blob/3.4.0/src/wrapper.js#L34

@mgol mgol closed this in #4361 Apr 17, 2019

mgol added a commit that referenced this issue Apr 17, 2019

Build: Fix unresolved jQuery reference in finalPropName
Also, prevent further similar breakages by changing our ESLint configuration
to disallow relying on a global jQuery object in AMD modules.

Fixes gh-4358
Closes gh-4361

mgol added a commit that referenced this issue Apr 17, 2019

Build: Fix unresolved jQuery reference in finalPropName
Also, prevent further similar breakages by changing our ESLint configuration
to disallow relying on a global jQuery object in AMD modules.

(cherry-picked from 8740305)

Fixes gh-4358
Closes gh-4361

@mgol mgol assigned mgol and unassigned dmethvin Apr 17, 2019

@GulajavaMinistudio GulajavaMinistudio referenced this issue Apr 20, 2019

Merged

ImageShader Additional Constructors #86

0 of 4 tasks complete
@timmywil

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.