[CLEANUP beta] Remove jQuery from build #19677

simonihmig · 2021-07-22T20:26:33Z

Removes jQuery from dependencies, the build process and test runs.

Should be passing once the other open jQuery related PRs have landed (#19666, #19675, #19676)

nlfurniss · 2021-08-20T02:56:34Z

@simonihmig this diff should fix the eslint issues and after rebasing the tests should pass as well

diff --git a/ember-cli-build.js b/ember-cli-build.js
index cdf3daf58..5c92e7a61 100644
--- a/ember-cli-build.js
+++ b/ember-cli-build.js
@@ -14,7 +14,6 @@ Error.stackTraceLimit = Infinity;
 
 const {
   routerES,
-  jquery,
   loader,
   qunit,
   handlebarsES,
diff --git a/lib/index.js b/lib/index.js
index 19bf17a76..778268f56 100644
--- a/lib/index.js
+++ b/lib/index.js
@@ -3,7 +3,6 @@
 const MergeTrees = require('broccoli-merge-trees');
 const Funnel = require('broccoli-funnel');
 const path = require('path');
-const resolve = require('resolve');
 const concatBundle = require('./concat-bundle');
 const buildDebugMacroPlugin = require('./build-debug-macro-plugin');
 const buildStripClassCallcheckPlugin = require('./build-strip-class-callcheck-plugin');

simonihmig · 2021-08-20T10:42:35Z

this diff should fix the eslint issues and after rebasing the tests should pass as well

Yeah, thanks @nlfurniss! Ready for Review! @mixonic

mixonic

Close! I kicked off a rebuild to get around browserstack issues. This is very exciting.

mixonic · 2021-08-22T00:11:43Z

lib/index.js

-      this.ui.writeWarnLine(
-        'Setting the `jquery-integration` optional feature flag to `true`, or not providing a setting at all, has been deprecated. You must add the `@ember/optional-features` addon and set this feature to `false`. This warning will become an error in Ember 4.0.0.\n\nFor more information, see the deprecation guide: https://deprecations.emberjs.com/v3.x/#toc_optional-feature-jquery-integration'
+      const SilentError = require('silent-error');
+      throw new SilentError(


This is the right thing to do? Or should we assert if anyone sets this feature enabled/disabled at all? As in, they should just remove it from their configuration.

The approach of require false is certainly more gentle, but a new 4.0 user would probably look curiously at a requirement to set a feature to disabled which... could not possibly be enabled.

Hm, right, I actually meant to only throw when this is explicitly set to true. I (wrongly) assumed this is already false by default, but it is not. So I will need to revisit this, to make it match RFC705 Phase 2. So I would basically use isFeatureExplicitlySet to check if is explicitly set to true, or do noting (optional feature has no effect) otherwise, right?

I have addressed this now!

simonihmig · 2021-08-22T23:11:42Z

I kicked off a rebuild to get around browserstack issues

Seems FF and Browserstack tests are still failing, so something real? 🤔 Wouldn't know why though. libEGL missing, does that ring a bell for you/someone?

simonihmig · 2021-09-24T08:47:47Z

tests/index.html

@@ -134,7 +126,7 @@
          testsTotal = testsPassed = testsFailed = 0;

          if (QUnit.urlParams.hideskipped) {
-            $('#qunit-tests').addClass('hideskipped');
+            document.getElementById('qunit-tests').classList.add('hideskipped');


This was the reason for failing FF and Browserstack tests! They used hideskipped in testem.ci-browsers.js, which was still jQuery based here. Still don't understand why this gave us these obscure error messages...

Good catch!!!

simonihmig · 2021-09-24T08:52:58Z

This took too long to make it green, sorry about that. But it finally passes all tests! 🎉

I was able to reproduce the failing Firefox CI job failure and fix it, see the inline comments... so @mixonic this is ready now for final review!

rwjblue

Thanks @simonihmig! Only one minor comment left...

rwjblue · 2021-09-27T16:28:28Z

lib/index.js

+    ) {
+      const SilentError = require('silent-error');
+      throw new SilentError(
+        'Setting the `jquery-integration` optional feature flag to `true` was deprecated in Ember 3.x and removed in Ember 4.0.0. You must add the `@ember/optional-features` addon and set this feature to `false`.\n\nFor more information, see the deprecation guide: https://deprecations.emberjs.com/v3.x/#toc_optional-feature-jquery-integration'


I think this prose needs work now that the logic has changed. We don't want folks installing the optional features addon at all, we want them to remove the configuration from their optional-features.json file...

Hm, ok, I can change that. But I vaguely remember in some past conversations (around jQuery RFCs) that the JSON file was considered an implementation details that users should not directly interact with, rather the "public API" is to use the commands provided by the addon, right? However there is no "remove" command AFAIK. So should I indeed point users to edit the optional-features.json file directly?

Personally, I am not too much worried about this, just want to make sure...

@rwjblue ping 👆 🙂

I'm not inclined to block on getting this dialed in exactly. It seems like we do lack some kind of followup logic for optional features- an ability to see what current settings match the ideal state and GC. Regardless setting to false is harmless.

I'm happy to see another PR with further work here (or maybe even across Ember and the optional features repo), but I'm going to land this patch so we can get it into betas ASAP.

rwjblue · 2021-09-27T16:28:57Z

tests/index.html

@@ -134,7 +126,7 @@
          testsTotal = testsPassed = testsFailed = 0;

          if (QUnit.urlParams.hideskipped) {
-            $('#qunit-tests').addClass('hideskipped');
+            document.getElementById('qunit-tests').classList.add('hideskipped');


Good catch!!!

Removes jQuery from dependencies, the build process and test runs.

…licitly enabled.

mixonic · 2021-10-27T17:51:47Z

I've force pushed a rebase.

mixonic · 2021-10-27T19:52:44Z

Thank you @simonihmig! This is incredible work.

simonihmig · 2021-10-28T10:20:04Z

Thanks @mixonic, happy to have helped putting an end to this looong story (jQuery removal) 😀

There is still a bit of jQuery cruft in comments and API docs. Nothing which is super important to fix for the major release I think. I can do that when I find the time, but if someone else reads this and wants to contribute: just search for jQuery across the repo...

simonihmig force-pushed the jquery-bundling branch from 1689cfb to 46c0b2e Compare August 20, 2021 10:37

simonihmig marked this pull request as ready for review August 20, 2021 10:42

simonihmig changed the title ~~Remove jQuery from build~~ [CLEANUP beta] Remove jQuery from build Aug 20, 2021

mixonic reviewed Aug 22, 2021

View reviewed changes

simonihmig force-pushed the jquery-bundling branch from 46c0b2e to 8a8b8ce Compare September 22, 2021 17:00

simonihmig commented Sep 24, 2021

View reviewed changes

rwjblue reviewed Sep 27, 2021

View reviewed changes

kategengler added the 4.0 Cleanup label Oct 12, 2021

simonihmig added 3 commits October 27, 2021 13:51

Remove jQuery from build

0d31f3a

Removes jQuery from dependencies, the build process and test runs.

Fix hideskipped QUnit param, refactoring jQuery implementation

3a5d028

Fix jquery-integration optional feature flag to only throw when exp…

52413c2

…licitly enabled.

mixonic force-pushed the jquery-bundling branch from 49ae891 to 52413c2 Compare October 27, 2021 17:51

mixonic approved these changes Oct 27, 2021

View reviewed changes

mixonic merged commit 5a36048 into emberjs:master Oct 27, 2021

simonihmig deleted the jquery-bundling branch October 28, 2021 10:08

nlfurniss mentioned this pull request Oct 28, 2021

[DOC beta] Remove some jQuery examples from comments #19807

Merged

simonihmig mentioned this pull request Dec 20, 2021

RFC #0386 - Tracking for Remove jQuery by Default emberjs/rfc-tracking#3

Closed

24 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CLEANUP beta] Remove jQuery from build #19677

[CLEANUP beta] Remove jQuery from build #19677

simonihmig commented Jul 22, 2021

nlfurniss commented Aug 20, 2021

simonihmig commented Aug 20, 2021

mixonic left a comment

mixonic Aug 22, 2021

simonihmig Aug 22, 2021

simonihmig Sep 24, 2021

simonihmig commented Aug 22, 2021

simonihmig Sep 24, 2021

rwjblue Sep 27, 2021

simonihmig commented Sep 24, 2021

rwjblue left a comment

rwjblue Sep 27, 2021

simonihmig Sep 28, 2021

simonihmig Oct 23, 2021

mixonic Oct 27, 2021

rwjblue Sep 27, 2021

mixonic commented Oct 27, 2021

mixonic commented Oct 27, 2021

simonihmig commented Oct 28, 2021

[CLEANUP beta] Remove jQuery from build #19677

[CLEANUP beta] Remove jQuery from build #19677

Conversation

simonihmig commented Jul 22, 2021

nlfurniss commented Aug 20, 2021

simonihmig commented Aug 20, 2021

mixonic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonihmig commented Aug 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonihmig commented Sep 24, 2021

rwjblue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mixonic commented Oct 27, 2021

mixonic commented Oct 27, 2021

simonihmig commented Oct 28, 2021