Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add support for TSLint #5697

Closed
wants to merge 10 commits into from
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
42 changes: 42 additions & 0 deletions packages/react-scripts/config/tslint.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
{
"defaultSeverity": "warning",
"rulesDirectory": "tslint-microsoft-contrib",

Choose a reason for hiding this comment

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

tslint-config-prettier and tslint-react would be nice defaults to have 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andyrichardson which rules were you thinking from tslint-react? A lot of the rules seemed to be style prefer nces which we try to avoid.

Choose a reason for hiding this comment

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

Fair point - jsx-key is already flagged by React so I guess the only other useful non-style option is jsx-self-close.

Copy link

@mohamedmansour mohamedmansour Nov 6, 2018

Choose a reason for hiding this comment

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

This will expose tslint.json to the root, so technically we can add our own rules to the project, correct? The current way create-react-app does eslint, it exposes a package.json config called eslintConfig, we could do the same and name it tslintConfig.

Copy link
Contributor

@Timer Timer Nov 6, 2018

Choose a reason for hiding this comment

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

@mohamedmansour just like ESLint, you will not be able to add your own rules. Added rules will strictly affect the editor output, not the build tools.

If you need to check linting on commit, I suggest you use a precommit hook.

Copy link

@mohamedmansour mohamedmansour Nov 6, 2018

Choose a reason for hiding this comment

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

This is unfortunate, why couldn't we change that model and allow the users to override tslint.json? The beautiful thing about TypeScript is that it is a superset of JavaScript, each team can decide what kind of strictness they need or want. Once create-react-app enforce this projects strictness for linting, then I cannot use the rules that I want to enforce in my own project.

For example, we are overriding many rules to fit our teams standard here https://github.com/OfficeDev/office-ui-fabric-react/blob/master/packages/tslint-rules/tslint.json

And in my current project, I just do the following in tslint.json:

{
  "extends": [
    "@uifabric/tslint-rules"
  ],
  "rules": {
    "jsx-no-lambda": false,
    "jsx-ban-props": false,
    "typedef": [false]
  }
}

Having a choice on how linting works is more preferable than the way this PR does it.

"rules": {
// https://palantir.github.io/tslint/rules/
"await-promise": true,
"new-parens": true,
"no-angle-bracket-type-assertion": true,
"no-conditional-assignment": true,
"no-debugger": true,
"no-duplicate-super": true,
"no-duplicate-switch-case": true,
"no-duplicate-variable": true,
"no-eval": true,
"no-floating-promises": true,
"no-for-in-array": true,
"no-implicit-dependencies": [true, "dev"],
"no-invalid-template-strings": true,
"no-invalid-this": true,
"no-namespace": true,
"no-sparse-arrays": true,
"no-string-throw": true,
"no-switch-case-fall-through": true,
"no-unused-expression": [true, "allow-fast-null-checks"],
// DEPRECATED. Recommended to use TS 'noUnusedLocals' for now
// "no-unused-variable": true,
"triple-equals": true,
"use-isnan": true,

// https://github.com/Microsoft/tslint-microsoft-contrib
"react-a11y-anchors": true,
"react-a11y-aria-unsupported-elements": true,
"react-a11y-event-has-role": true,
"react-a11y-image-button-has-alt": true,
"react-a11y-img-has-alt": true,
"react-a11y-props": true,
"react-a11y-proptypes": true,
"react-a11y-role": true,
"react-a11y-role-has-required-aria-props": true,
"react-a11y-role-supports-aria-props": true
}
}
1 change: 1 addition & 0 deletions packages/react-scripts/config/webpack.config.dev.js
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,7 @@ module.exports = {
async: false,
checkSyntacticErrors: true,
tsconfig: paths.appTsConfig,
tslint: path.resolve(__dirname, 'tslint.json'),
compilerOptions: {
module: 'esnext',
moduleResolution: 'node',
Expand Down
1 change: 1 addition & 0 deletions packages/react-scripts/config/webpack.config.prod.js
Original file line number Diff line number Diff line change
Expand Up @@ -544,6 +544,7 @@ module.exports = {
async: false,
checkSyntacticErrors: true,
tsconfig: paths.appTsConfig,
tslint: path.resolve(__dirname, 'tslint.json'),
compilerOptions: {
module: 'esnext',
moduleResolution: 'node',
Expand Down
2 changes: 2 additions & 0 deletions packages/react-scripts/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@
"sass-loader": "7.1.0",
"style-loader": "0.23.0",
"terser-webpack-plugin": "1.1.0",
"tslint": "5.11.0",
"tslint-microsoft-contrib": "5.2.1",
"url-loader": "1.1.1",
"webpack": "4.19.1",
"webpack-dev-server": "3.1.9",
Expand Down
1 change: 1 addition & 0 deletions packages/react-scripts/template-typescript/src/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,5 @@ ReactDOM.render(<App />, document.getElementById('root'));
// If you want your app to work offline and load faster, you can change
// unregister() to register() below. Note this comes with some pitfalls.
// Learn more about service workers: http://bit.ly/CRA-PWA
// tslint:disable-next-line:no-floating-promises
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Timer let me know how you feel about this. I refactored all the serviceWorker code to use async/await, which fixes all the TSLint errors.

serviceWorker.unregister();
149 changes: 72 additions & 77 deletions packages/react-scripts/template-typescript/src/serviceWorker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,105 +39,100 @@ export function register(config?: Config) {
return;
}

window.addEventListener('load', () => {
window.addEventListener('load', async () => {
const swUrl = `${process.env.PUBLIC_URL}/service-worker.js`;

if (isLocalhost) {
// This is running on localhost. Let's check if a service worker still exists or not.
checkValidServiceWorker(swUrl, config);
await checkValidServiceWorker(swUrl, config);

// Add some additional logging to localhost, pointing developers to the
// service worker/PWA documentation.
navigator.serviceWorker.ready.then(() => {
console.log(
'This web app is being served cache-first by a service ' +
'worker. To learn more, visit http://bit.ly/CRA-PWA'
);
});
await navigator.serviceWorker.ready;

console.log(
'This web app is being served cache-first by a service ' +
'worker. To learn more, visit http://bit.ly/CRA-PWA'
);
} else {
// Is not localhost. Just register service worker
registerValidSW(swUrl, config);
await registerValidSW(swUrl, config);
}
});
}
}

function registerValidSW(swUrl: string, config?: Config) {
navigator.serviceWorker
.register(swUrl)
.then(registration => {
registration.onupdatefound = () => {
const installingWorker = registration.installing;
if (installingWorker == null) {
return;
}
installingWorker.onstatechange = () => {
if (installingWorker.state === 'installed') {
if (navigator.serviceWorker.controller) {
// At this point, the updated precached content has been fetched,
// but the previous service worker will still serve the older
// content until all client tabs are closed.
console.log(
'New content is available and will be used when all ' +
'tabs for this page are closed. See http://bit.ly/CRA-PWA.'
);

// Execute callback
if (config && config.onUpdate) {
config.onUpdate(registration);
}
} else {
// At this point, everything has been precached.
// It's the perfect time to display a
// "Content is cached for offline use." message.
console.log('Content is cached for offline use.');

// Execute callback
if (config && config.onSuccess) {
config.onSuccess(registration);
}
async function registerValidSW(swUrl: string, config?: Config) {
try {
const registration = await navigator.serviceWorker.register(swUrl);

registration.onupdatefound = () => {
const installingWorker = registration.installing;
if (!installingWorker) {
return;
}
installingWorker.onstatechange = () => {
if (installingWorker.state === 'installed') {
if (navigator.serviceWorker.controller) {
// At this point, the updated precached content has been fetched,
// but the previous service worker will still serve the older
// content until all client tabs are closed.
console.log(
'New content is available and will be used when all ' +
'tabs for this page are closed. See http://bit.ly/CRA-PWA.'
);

// Execute callback
if (config && config.onUpdate) {
config.onUpdate(registration);
}
} else {
// At this point, everything has been precached.
// It's the perfect time to display a
// "Content is cached for offline use." message.
console.log('Content is cached for offline use.');

// Execute callback
if (config && config.onSuccess) {
config.onSuccess(registration);
}
}
};
}
};
})
.catch(error => {
console.error('Error during service worker registration:', error);
});
};
} catch (error) {
console.error('Error during service worker registration:', error);
}
}

function checkValidServiceWorker(swUrl: string, config?: Config) {
async function checkValidServiceWorker(swUrl: string, config?: Config) {
// Check if the service worker can be found. If it can't reload the page.
fetch(swUrl)
.then(response => {
// Ensure service worker exists, and that we really are getting a JS file.
const contentType = response.headers.get('content-type');
if (
response.status === 404 ||
(contentType != null && contentType.indexOf('javascript') === -1)
) {
// No service worker found. Probably a different app. Reload the page.
navigator.serviceWorker.ready.then(registration => {
registration.unregister().then(() => {
window.location.reload();
});
});
} else {
// Service worker found. Proceed as normal.
registerValidSW(swUrl, config);
}
})
.catch(() => {
console.log(
'No internet connection found. App is running in offline mode.'
);
});
try {
const response = await fetch(swUrl);

// Ensure service worker exists, and that we really are getting a JS file.
const contentType = response.headers.get('content-type');
if (
response.status === 404 ||
(contentType && contentType.indexOf('javascript') === -1)
) {
// No service worker found. Probably a different app. Reload the page.
await unregister();
window.location.reload();
} else {
// Service worker found. Proceed as normal.as
await registerValidSW(swUrl, config);
}
} catch {
console.log(
'No internet connection found. App is running in offline mode.'
);
}
}

export function unregister() {
export async function unregister() {
if ('serviceWorker' in navigator) {
navigator.serviceWorker.ready.then(registration => {
registration.unregister();
});
const registration = await navigator.serviceWorker.ready;
await registration.unregister();
}
}
Loading