Skip to content

Remove legacy linting rules #48

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

Merged
merged 1 commit into from
Aug 27, 2018
Merged

Remove legacy linting rules #48

merged 1 commit into from
Aug 27, 2018

Conversation

ccnixon
Copy link
Contributor

@ccnixon ccnixon commented Aug 27, 2018

What does this PR do?
Removes the legacy linting rules and replaces them with a general rule to just flag any non-es5 syntax. This will allow us to use Standard as our code formatter while also protecting against accidentally shipping code that potentially breaks our supported browsers.

Are there breaking changes in this PR?

Has this been tested end-to-end? Please provide screenshots on how the fix now populates in the end tool. If not, what was done to test?

Any background context you want to provide?

Is there parity with the server-side/android/iOS integration (if applicable)?

Does this require a metadata change? If so, please link the PR from https://github.com/segmentio/destination-scripts.

What are the relevant tickets?

Link to CC ticket

List all the tests accounts you have used to make sure this change works

Helpful Docs

@ccnixon ccnixon requested review from a team August 27, 2018 16:15
@ccnixon ccnixon merged commit da0b549 into master Aug 27, 2018
@ccnixon ccnixon deleted the fix-linting branch August 27, 2018 16:43
@f2prateek
Copy link

for refernce, here's how we integrated prettier into analytics.js-core segmentio/analytics.js-core#89

@ccnixon
Copy link
Contributor Author

ccnixon commented Aug 27, 2018

Thanks @f2prateek - I like that. @fathyb - from what you can tell, does the legacy eslint config help protect us more than just setting the ecmaVersion to 3?

@ccnixon
Copy link
Contributor Author

ccnixon commented Aug 27, 2018

It's tough to find compatibility tables for ie8-10

@fathyb
Copy link
Contributor

fathyb commented Aug 27, 2018

@ccnixon

does the legacy eslint config help protect us more than just setting the ecmaVersion to 3?

Yes, mainly to cope with IE8 quircks:

  "rules": {
    "comma-dangle": ["error", "never"],
    "dot-notation": ["error", { "allowKeywords": false }],
    "no-catch-shadow": "error",
    "no-restricted-globals": ["error", "JSON"], // IE7
    "quote-props": ["error", "as-needed", { "keywords": true }],
    "radix": ["error", "always"]
  }
  • comma-dangle prevents you from using trailing commas in object literals (which are legal per ES3 but IE8 doesn't support it)
  • dot-notation will prevent you from using a keyword (ie. default) in a member expression
  • no-catch-shadow prevent a bug in IE8 when a catch arguments shadows a variable in the outer scope
  • no-restricted-globals to prevent using JSON (and use something like require('json') instead)
  • quote-props same as dot-notation but for object literals
  • radix because the behaviour of parseInt is inconsistent between ES3 and ES5

@Peripheral1994
Copy link

FWIW, we've formally deprecated support for IE8 and earlier, though I'm 👍 for continuing to informally support it if it isn't a blocker for us long-term. :)

@Peripheral1994
Copy link

e.g. We use Array.isArray in several destinations now on the clientside as a result, IIRC.

@ccnixon
Copy link
Contributor Author

ccnixon commented Aug 27, 2018

Great, thanks @fathyb!

nettofarah pushed a commit that referenced this pull request Feb 1, 2021
* add survicate

* add walkme

* Ignore tests for survicate and walkme
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants