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.

Sign up for GitHub

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

[CLEANUP beta] Remove jQuery from build #19677

Merged
merged 3 commits into from
Oct 27, 2021
Merged

[CLEANUP beta] Remove jQuery from build #19677

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0d31f3a
Remove jQuery from build
simonihmig Jul 22, 2021
3a5d028
Fix `hideskipped` QUnit param, refactoring jQuery implementation
simonihmig Sep 24, 2021
52413c2
Fix `jquery-integration` optional feature flag to only throw when exp…
simonihmig Sep 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions bin/run-tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,6 @@ function generateTestsFor(packageName) {
testFunctions.push(() => run('package=' + packageName + '&edition=classic'));
testFunctions.push(() => run('package=' + packageName + '&prebuilt=true'));
testFunctions.push(() => run('package=' + packageName + '&enableoptionalfeatures=true'));

// TODO: this should ultimately be deleted (when all packages can run with and
// without jQuery)
if (packageName !== 'ember') {
testFunctions.push(() => run('package=' + packageName + '&jquery=none'));
}
}

function generateEachPackageTests() {
Expand Down
8 changes: 0 additions & 8 deletions broccoli/packages.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,6 @@ module.exports.routerES = function _routerES() {
});
};

module.exports.jquery = function _jquery() {
return new Funnel(findLib('jquery'), {
files: ['jquery.js'],
destDir: 'jquery',
annotation: 'jquery',
});
};

module.exports.loader = function _loader() {
return new Funnel('packages/loader/lib', {
files: ['index.js'],
Expand Down
10 changes: 1 addition & 9 deletions ember-cli-build.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ Error.stackTraceLimit = Infinity;

const {
routerES,
jquery,
loader,
qunit,
handlebarsES,
Expand Down Expand Up @@ -249,14 +248,7 @@ function templateCompilerBundle(emberPackages, transpileTree) {
}

function testHarness() {
return new MergeTrees([
emptyTestem(),
testPolyfills(),
testIndexHTML(),
loader(),
qunit(),
jquery(),
]);
return new MergeTrees([emptyTestem(), testPolyfills(), testIndexHTML(), loader(), qunit()]);
}

function emptyTestem() {
Expand Down
59 changes: 11 additions & 48 deletions lib/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -33,7 +32,6 @@ function add(paths, name, path) {
add(paths, 'prod', 'vendor/ember/ember.js');
add(paths, 'debug', 'vendor/ember/ember.js');
add(paths, 'testing', 'vendor/ember/ember-testing.js');
add(paths, 'jquery', 'vendor/ember/jquery/jquery.js');

add(
absolutePaths,
Expand Down Expand Up @@ -65,7 +63,6 @@ module.exports = {
name: 'ember-source',
paths,
absolutePaths,
_jqueryIntegrationEnabled: true,
_overrideTree: undefined,

included() {
Expand Down Expand Up @@ -95,25 +92,6 @@ module.exports = {
);
}

if (
optionalFeaturesMissing ||
typeof optionalFeatures.isFeatureExplicitlySet !== 'function'
) {
message.push(
'* Unable to detect if jquery-integration is explicitly set to a value, please update `@ember/optional-features` to the latest version'
);
}

if (
optionalFeaturesMissing ||
(typeof optionalFeatures.isFeatureExplicitlySet === 'function' &&
!optionalFeatures.isFeatureExplicitlySet('jquery-integration'))
) {
message.push(
`* The jquery-integration optional feature should be explicitly set to a value under Octane, run \`ember feature:disable jquery-integration\` to disable it, or \`ember feature:enable jquery-integration\` to explicitly enable it`
);
}

if (
optionalFeaturesMissing ||
optionalFeatures.isFeatureEnabled('application-template-wrapper')
Expand Down Expand Up @@ -164,12 +142,15 @@ module.exports = {
}
}

this._jqueryIntegrationEnabled =
optionalFeaturesMissing || optionalFeatures.isFeatureEnabled('jquery-integration');

if (this._jqueryIntegrationEnabled) {
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'
if (
!optionalFeaturesMissing &&
optionalFeatures.isFeatureEnabled('jquery-integration') &&
typeof optionalFeatures.isFeatureExplicitlySet === 'function' &&
optionalFeatures.isFeatureExplicitlySet('jquery-integration')
) {
const SilentError = require('silent-error');
throw new SilentError(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have addressed this now!

'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'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rwjblue ping 👆 🙂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

);
}
},
Expand Down Expand Up @@ -237,10 +218,7 @@ module.exports = {
false
);

let exclude = [
isProduction ? 'ember-testing/**' : null,
!this._jqueryIntegrationEnabled ? 'jquery' : null,
].filter((value) => value !== null);
let exclude = isProduction ? ['ember-testing/**'] : [];

let emberFiles = new MergeTrees([new Funnel(packages, { exclude }), dependencies, headerFiles]);

Expand Down Expand Up @@ -270,21 +248,6 @@ module.exports = {
},

treeForVendor(tree) {
let jqueryPath;

try {
jqueryPath = path.dirname(
resolve.sync('jquery/package.json', { basedir: this.project.root })
);
} catch (error) {
jqueryPath = path.dirname(require.resolve('jquery/package.json'));
}

let jquery = new Funnel(jqueryPath + '/dist', {
destDir: 'ember/jquery',
files: ['jquery.js'],
});

let templateCompiler = new Funnel(tree, {
destDir: 'ember',
include: ['ember-template-compiler.js', 'ember-template-compiler.map'],
Expand Down Expand Up @@ -319,6 +282,6 @@ module.exports = {
});
}

return debugTree(new MergeTrees([ember, templateCompiler, jquery]), 'vendor:final');
return debugTree(new MergeTrees([ember, templateCompiler]), 'vendor:final');
},
};
5 changes: 1 addition & 4 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,7 @@
"name": "ember-source",
"version": "4.1.0-alpha.2",
"description": "A JavaScript framework for creating ambitious web applications",
"keywords": [
"ember-addon"
],
"keywords": ["ember-addon"],
"homepage": "https://emberjs.com/",
"bugs": {
"url": "https://github.com/emberjs/ember.js/issues"
Expand Down Expand Up @@ -69,7 +67,6 @@
"ember-cli-version-checker": "^5.1.1",
"ember-router-generator": "^2.0.0",
"inflection": "^1.13.1",
"jquery": "^3.5.1",
"resolve": "^1.17.0",
"semver": "^7.3.4",
"silent-error": "^1.1.1"
Expand Down
3 changes: 0 additions & 3 deletions packages/jquery/index.js
View file
Open in desktop

This file was deleted.

10 changes: 1 addition & 9 deletions tests/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,6 @@
// Close the script tag to make sure document.write happens
</script>

<script type="text/javascript">
// Fallback to default jQuery
if (jQueryVersion !== 'none' && !window.jQuery) {
loadScript('./jquery/jquery.js');
}
// Close the script tag to make sure document.write happens
</script>

<script>
(function() {
window.EmberENV = window.EmberENV || {};
Expand Down Expand Up @@ -134,7 +126,7 @@
testsTotal = testsPassed = testsFailed = 0;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!!!

}
});

Expand Down
5 changes: 0 additions & 5 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7812,11 +7812,6 @@ jmespath@0.15.0:
resolved "https://registry.yarnpkg.com/jmespath/-/jmespath-0.15.0.tgz#a3f222a9aae9f966f5d27c796510e28091764217"
integrity "sha1-o/Iiqarp+Wb10nx5ZRDigJF2Qhc= sha512-+kHj8HXArPfpPEKGLZ+kB5ONRTCiGQXo8RQYL0hH8t6pWXUBBK5KkkQmTNOwKK4LEsd0yTsgtjJVm4UBSZea4w=="

jquery@^3.5.1:
version "3.5.1"
resolved "https://registry.yarnpkg.com/jquery/-/jquery-3.5.1.tgz#d7b4d08e1bfdb86ad2f1a3d039ea17304717abb5"
integrity sha512-XwIBPqcMn57FxfT+Go5pzySnm4KWkT1Tv7gjrpT1srtf8Weynl6R273VJ5GjkRb51IzMp5nbaPjJXMWeju2MKg==

js-reporters@1.2.3:
version "1.2.3"
resolved "https://registry.yarnpkg.com/js-reporters/-/js-reporters-1.2.3.tgz#8febcab370539df62e09b95da133da04b11f6168"
Expand Down