Skip to content

ci: update browsers tests to test supported browsers only #16855

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

Merged
dgp1130 merged 2 commits into from
Feb 11, 2020
Merged

ci: update browsers tests to test supported browsers only #16855

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e501cf3
ci: update browsers tests to test supported browsers only
alan-agius4 Feb 7, 2020
fea2ef3
refactor: remove safari nomodule polyfills as it's unsupported
alan-agius4 Feb 7, 2020
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
19 changes: 0 additions & 19 deletions packages/angular_devkit/build_angular/src/angular-cli-files/models/safari-nomodule.js
View file
Open in desktop

This file was deleted.

13 changes: 0 additions & 13 deletions packages/angular_devkit/build_angular/src/angular-cli-files/models/webpack-configs/common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,19 +132,6 @@ export function getCommonConfig(wco: WebpackConfigOptions): Configuration {
buildOptions.es5BrowserSupport ||
(buildOptions.es5BrowserSupport === undefined && buildBrowserFeatures.isEs5SupportNeeded())
) {
// The nomodule polyfill needs to be inject prior to any script and be
// outside of webpack compilation because otherwise webpack will cause the
// script to be wrapped in window["webpackJsonp"] which causes this to fail.
if (buildBrowserFeatures.isNoModulePolyfillNeeded()) {
const noModuleScript: ExtraEntryPoint = {
bundleName: 'polyfills-nomodule-es5',
input: path.join(__dirname, '..', 'safari-nomodule.js'),
};
buildOptions.scripts = buildOptions.scripts
? [...buildOptions.scripts, noModuleScript]
: [noModuleScript];
}

const polyfillsChunkName = 'polyfills-es5';
entryPoints[polyfillsChunkName] = [path.join(__dirname, '..', 'es5-polyfills.js')];
if (differentialLoadingMode) {
Expand Down
10 changes: 5 additions & 5 deletions ...lar_devkit/build_angular/src/angular-cli-files/utilities/index-file/augment-index-html.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,17 +148,17 @@ export async function augmentIndexHtml(params: AugmentIndexHtmlOptions): Promise
// such as runtime.js
const scriptPredictor = ({ file }: FileInfo): boolean => file === script;
if (!files.some(scriptPredictor)) {
// in some cases for differential loading file with the same name is avialable in both
// in some cases for differential loading file with the same name is available in both
// nomodule and module such as scripts.js
// we shall not add these attributes if that's the case
const isNoModuleType = noModuleFiles.some(scriptPredictor);
const isModuleType = moduleFiles.some(scriptPredictor);

if (isNoModuleType && !isModuleType) {
attrs.push({ name: 'nomodule', value: null });
if (!script.startsWith('polyfills-nomodule-es5')) {
attrs.push({ name: 'defer', value: null });
}
attrs.push(
{ name: 'nomodule', value: null },
{ name: 'defer', value: null },
);
} else if (isModuleType && !isNoModuleType) {
attrs.push({ name: 'type', value: 'module' });
} else {
Expand Down
1 change: 0 additions & 1 deletion packages/angular_devkit/build_angular/src/angular-cli-files/utilities/package-chunk-sort.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ export function generateEntryPoints(appConfig: {
};

const entryPoints = [
'polyfills-nomodule-es5',
'runtime',
'polyfills-es5',
'polyfills',
Expand Down
4 changes: 1 addition & 3 deletions packages/angular_devkit/build_angular/src/browser/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,9 +383,7 @@ export function buildWebpackBrowser(
}

// All files at this point except ES5 polyfills are module scripts
const es5Polyfills =
file.file.startsWith('polyfills-es5') ||
file.file.startsWith('polyfills-nomodule-es5');
const es5Polyfills = file.file.startsWith('polyfills-es5');
if (!es5Polyfills) {
moduleFiles.push(file);
}
Expand Down
19 changes: 0 additions & 19 deletions packages/angular_devkit/build_angular/src/utils/build-browser-features.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,25 +37,6 @@ export class BuildBrowserFeatures {
return !this.isFeatureSupported('es6-module');
}

/**
* Safari 10.1 and iOS Safari 10.3 supports modules,
* but does not support the `nomodule` attribute.
* While return `true`, when support for Safari 10.1 and iOS Safari 10.3
* is required and in differential loading is enabled.
*/
isNoModulePolyfillNeeded(): boolean {
if (!this.isDifferentialLoadingNeeded()) {
return false;
}

const safariBrowsers = [
'safari 10.1',
'ios_saf 10.3',
];

return this.supportedBrowsers.some(browser => safariBrowsers.includes(browser));
}

/**
* True, when a browser feature is supported partially or fully.
*/
Expand Down
65 changes: 0 additions & 65 deletions packages/angular_devkit/build_angular/src/utils/build-browser-features_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,69 +127,4 @@ describe('BuildBrowserFeatures', () => {
expect(buildBrowserFeatures.isFeatureSupported('es6-module')).toBe(true);
});
});

describe('isNoModulePolyfillNeeded', () => {
it('should be false for Safari 10.1 when target is ES5', () => {
host.writeMultipleFiles({
'browserslist': 'Safari 10.1',
});

const buildBrowserFeatures = new BuildBrowserFeatures(
workspaceRootSysPath,
ScriptTarget.ES5,
);
expect(buildBrowserFeatures.isNoModulePolyfillNeeded()).toBe(false);
});

it('should be false for Safari 10.1 when target is ES2015', () => {
host.writeMultipleFiles({
'browserslist': 'Safari 10.1',
});

const buildBrowserFeatures = new BuildBrowserFeatures(
workspaceRootSysPath,
ScriptTarget.ES2015,
);
expect(buildBrowserFeatures.isNoModulePolyfillNeeded()).toBe(false);
});

it('should be true for Safari 9+ when target is ES2015', () => {
host.writeMultipleFiles({
'browserslist': 'Safari >= 9',
});

const buildBrowserFeatures = new BuildBrowserFeatures(
workspaceRootSysPath,
ScriptTarget.ES2015,
);
expect(buildBrowserFeatures.isNoModulePolyfillNeeded()).toBe(true);
});

it('should be false for Safari 9+ when target is ES5', () => {
host.writeMultipleFiles({
'browserslist': 'Safari >= 9',
});

const buildBrowserFeatures = new BuildBrowserFeatures(
workspaceRootSysPath,
ScriptTarget.ES5,
);
expect(buildBrowserFeatures.isNoModulePolyfillNeeded()).toBe(false);
});

it('should be false when not supporting Safari 10.1 target is ES2015', () => {
host.writeMultipleFiles({
'browserslist': `
Edge 18
IE 9
`,
});

const buildBrowserFeatures = new BuildBrowserFeatures(
workspaceRootSysPath,
ScriptTarget.ES2015,
);
expect(buildBrowserFeatures.isNoModulePolyfillNeeded()).toBe(false);
});
});
});
47 changes: 8 additions & 39 deletions tests/legacy-cli/e2e/assets/protractor-saucelabs.conf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,33 +19,17 @@ exports.config = {
multiCapabilities: [
{
browserName: 'chrome',
version: '41',
version: '80',
tunnelIdentifier,
},
{
browserName: 'chrome',
version: '75',
tunnelIdentifier,
},
{
browserName: 'safari',
platform: 'OS X 10.11',
version: '9.0',
tunnelIdentifier,
},
{
browserName: 'safari',
platform: 'OS X 10.12',
version: '10.1',
Copy link
Copy Markdown
Member

@clydin clydin Feb 6, 2020

Choose a reason for hiding this comment

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

This was here to ensure that the nomodule polyfill works.

Copy link
Copy Markdown
Collaborator Author

@alan-agius4 alan-agius4 Feb 6, 2020

Choose a reason for hiding this comment

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

I asked @IgorMinar if we should drop the polyfill in the first place since it’s an unsupported browser.

Copy link
Copy Markdown
Contributor

@IgorMinar IgorMinar Feb 6, 2020

Choose a reason for hiding this comment

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

Can we test the nomodules polyfill on ie?

Copy link
Copy Markdown
Member

@clydin clydin Feb 7, 2020

Choose a reason for hiding this comment

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

The polyfill is only used on Safari 10.1 (iOS Safari 10.3). That specific version is in the unique situation of supporting module scripts but not the nomodule attribute. This causes both sets of scripts (ES5 & ES2015) to be executed on page load. Both the version prior (10.0) and the version after (11.0) do not have this issue. Since 10.1 is technically out of the support range, the polyfill could be dropped and the polyfill script could be manually included by any application attempting to support 10.1. (script link: https://gist.github.com/samthor/64b114e4a4f539915a95b91ffd340acc)

Copy link
Copy Markdown
Contributor

@IgorMinar IgorMinar Feb 8, 2020

Choose a reason for hiding this comment

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

sounds good to me. let's drop it.

// This specific version is needed as otherwise it will not pass
// See: https://github.com/angular/angular-cli/issues/15084
seleniumVersion: '3.4.0',
browserName: 'firefox',
version: '72',
tunnelIdentifier,
},
{
browserName: 'safari',
platform: 'macOS 10.13',
version: '11.1',
browserName: 'firefox',
version: '68', // Latest Firefox ESR version
tunnelIdentifier,
},
{
Expand All @@ -55,18 +39,9 @@ exports.config = {
tunnelIdentifier,
},
{
browserName: 'firefox',
version: '48',
tunnelIdentifier,
},
{
browserName: 'firefox',
version: '60',
tunnelIdentifier,
},
{
browserName: 'firefox',
version: '68',
Copy link
Copy Markdown
Member

@clydin clydin Feb 6, 2020

Choose a reason for hiding this comment

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

Latest Firefox ESR version is 68: https://www.mozilla.org/en-US/firefox/68.4.2/releasenotes/

Copy link
Copy Markdown
Collaborator Author

@alan-agius4 alan-agius4 Feb 7, 2020

Choose a reason for hiding this comment

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

We don't seem to support FireFox ESR https://angular.io/guide/browser-support#browser-support

Copy link
Copy Markdown
Member

@clydin clydin Feb 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I’d consider it technically a latest version of Firefox; making Firefox essentially have two. The ESR latest version is used predominantly in corporate environments (intranet applications, etc.) due to its support and stability guarantees.
There have been reported issues in the past concerning support of previous ESR versions.

@IgorMinar thoughts?

Copy link
Copy Markdown
Contributor

@IgorMinar IgorMinar Feb 8, 2020

Choose a reason for hiding this comment

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

I'm fine adding it, but then we should also document it and add it to the test suite no the framework repo.

browserName: 'safari',
platform: 'macOS 10.15',
version: '13.0',
tunnelIdentifier,
},
{
Expand All @@ -81,12 +56,6 @@ exports.config = {
version: '11',
tunnelIdentifier,
},
{
browserName: "MicrosoftEdge",
platform: 'Windows 10',
version: "14.14393",
tunnelIdentifier,
},
{
browserName: "MicrosoftEdge",
platform: 'Windows 10',
Expand Down
59 changes: 0 additions & 59 deletions tests/legacy-cli/e2e/tests/misc/support-safari-10.1.ts
View file
Open in desktop

This file was deleted.