closes LA-995
flag=none
1. unify the instui ui-babel-preset options across sub-packages
2. include "@instructure" as part of each sub-package name
3. unify the commonjs and es module directory structure
4. use index.js to export anything needed to be imported from
outside the package. This removes any dependency on the
package's internal directory structure and makes importing
independent of commonjs vs es module simple
5. move canvas-rce's main entry from async.js to index.js
(I'm 98% sure index.js was an artifact of the old rce)
test plan:
- canvas-lms builds locally and in docker. (Jenkins builds
the docker image, so I presume if it passes jenkins,
the docker build wasn't broken)
- sniff test the result, esp. student planner and the new rce.
- yarn build:watch from canvas-rce, canvas-planner, and
canvas-media packages should work, and yarn build:watch
from canvas-lms should pick-up any changes.
(I've verified this by finding a console.log statement
and changing the message then use devtools to see it's in
the new source after refreshing the browser)
Change-Id: I5f242d06ad655597cd416057d4b740d4bd86003b
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/238603
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Alex Anderson <raanderson@instructure.com>
QA-Review: Robin Kuss <rkuss@instructure.com>
Product-Review: Ed Schiebel <eschiebel@instructure.com>
This makes it so all staged *.js files are formatted using
prettier as a precommit hook rather than just selected
portions.
Test Plan:
- Commit a JS file with something not pretty in it.
- It should be pretty after the commit.
closes COREFE-347
closes COREFE-50
flag = none
Change-Id: Id9ad77e68fd95720752a316e5f2111ab6c9a02bc
Reviewed-on: https://gerrit.instructure.com/212895
Tested-by: Jenkins
Reviewed-by: Brent Burgoyne <bburgoyne@instructure.com>
QA-Review: Clay Diffrient <cdiffrient@instructure.com>
Product-Review: Clay Diffrient <cdiffrient@instructure.com>
We agree that people _should_ not use PropTypes.object normally, but
there _are_ times where doing that makes more sense then trying to come
up with a PropTypes.shape to use instead. So this is something we think
should be enforced in code-review rather than -2ing commits because
of it. We’d rather get rid of the rule than see a bunch of
`eslint-disable-next-line react/forbid-prop-types` scattered around
trying to ignore it where people knew what they were doing.
Test plan:
* you shouldn’t see any more warnings about
`PropTypes.object is forbidden` when you eslint
Change-Id: Icc9071604a162260aa9bce3a916f046f2553b66a
Reviewed-on: https://gerrit.instructure.com/209451
Tested-by: Jenkins
Reviewed-by: Ed Schiebel <eschiebel@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
this disallows things like creating 2 react classes in one file
like we do in app/jsx/shared/TermsOfServiceModal.js
I don’t see how that is going to catch any bugs and I don’t see why
we should tell people they can’t do that so I vote turn this off
test plan:
if you agree +2, if not, that’s fine and I’ll abandon
Change-Id: I0751609a75051e379f958aaed2e1c4dd260c020d
Reviewed-on: https://gerrit.instructure.com/207306
Tested-by: Jenkins
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
test plan:
- node ./node_modules/.bin/eslint packages/canvas-rce/src/bridge/Bridge.js
does not generate a warning or error.
Change-Id: I136f9bce39ea9a85999cbd5b244fd9d021b3a123
Reviewed-on: https://gerrit.instructure.com/205915
Reviewed-by: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
Tested-by: Jenkins
The previous commit makes it so i18nliner can understand <> instead of
<React.Fragment>, so this turns back on the eslint rule we had to turn
Off
Test plan:
* run bin/rake i18n:generate_js (or let the Jenkins builds run)
* it should pass
Change-Id: Ia3204cd6c3147eb52ca612adbb3b20b2df8c6921
Reviewed-on: https://gerrit.instructure.com/205378
Tested-by: Jenkins
Reviewed-by: Ed Schiebel <eschiebel@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
I wouldn't mind this rule, but it tries to force you into syntax that
i18nliner can't support.
Change-Id: Id6238223850ec134dd9aea1ddf34e1713f288e39
Reviewed-on: https://gerrit.instructure.com/205307
Reviewed-by: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
Tested-by: Jenkins
instead of having our own eslint in canvas-rce, and maintaining all
future rules like react hooks and stuff we can just use the same one
that the rest of the repo uses.
test plan:
* cd packages/canvas-rce
* `yarn lint` should run
Change-Id: Idbe8af533f7a19035d1998538d2d6fd1a711f164
Reviewed-on: https://gerrit.instructure.com/204998
Tested-by: Jenkins
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
QA-Review: Clay Diffrient <cdiffrient@instructure.com>
Product-Review: Clay Diffrient <cdiffrient@instructure.com>
AirBnB’s styleguide wants to force you to write
“foo” + bar
as
`foo${bar}`
see: https://eslint.org/docs/rules/prefer-template
I agree that the former is probably easier to read but I don’t know that
I care that much about to _force_ it. Sometimes I see things that use
“+” that seem nicer to read than the ones that use template literals.
and I just don’t think this is going to catch any bugs either way.
what say ye?
- 1 this if you want to keep caring about it
+2 this if you agree we should stop caring
Change-Id: Ie2fd6267dd0566da73034f7a62bd9dd73ebf4138
Reviewed-on: https://gerrit.instructure.com/205253
Tested-by: Jenkins
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
this commit increases the error-level on prop types from warning to
error.
it doesn't alert on components that don't have proptypes declared.
Change-Id: I6f74f1f6f3b3e0867002199e5507466038d8e1c3
Reviewed-on: https://gerrit.instructure.com/204773
Tested-by: Jenkins
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
QA-Review: Clay Diffrient <cdiffrient@instructure.com>
Product-Review: Clay Diffrient <cdiffrient@instructure.com>
We frequently have code like:
const {id, children} = props
const otherProps = omitProps(props, null, ['id', 'value', 'onChange'])
you could rewrite that as:
const {id, children, value, onChange, ...otherProps} = this.props
But if you did that right now, without this change, you’d get an eslint
error about `no-unused-vars` because you didn’t use value or onChange
in this function.
This makes it so eslint doesn’t complain about that.
See: https://eslint.org/docs/rules/no-unused-vars#ignorerestsiblings
Test plan:
* So the question is:
* Do you think that complaining about this rule actually catches any
real bugs?
* if so, -2 this.
* if not, +2 it
Change-Id: I2fc9b351f34e80ec74199569f768ccc71dfcd705
Reviewed-on: https://gerrit.instructure.com/202669
Tested-by: Jenkins
Reviewed-by: Ed Schiebel <eschiebel@instructure.com>
QA-Review: Ed Schiebel <eschiebel@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
Update the react dependency to 16.8 since we're starting to use react
hooks, which requires that version at minimum.
test plan:
- linting still works
- intentionally create a violation of rules-of-hooks. Linting should
find and report the error.
Change-Id: I0baccd64397b31118b96eb390462acf56daeca96
Reviewed-on: https://gerrit.instructure.com/199053
Reviewed-by: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
Tested-by: Jenkins
…instead, just use the default config for the rest of the repo
Test plan:
* cd packages/canvas-planner
* yarn eslint src/components/BadgeList/index.js
* you should not see an error about not being able to find flowtype
Change-Id: I48c4ab3ac20a60352c20c489addb4d4b4a32e83a
Reviewed-on: https://gerrit.instructure.com/199060
Reviewed-by: Jon Willesen <jonw+gerrit@instructure.com>
QA-Review: Jon Willesen <jonw+gerrit@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
Tested-by: Jenkins
This only affects javascript files and only affects things
from the prettier whitelist
This will run slowly if you don't have node_modules installed
locally (e.g., in Docker), but it will gladly attempt to
run things in Docker for you.
This adds a new githook_installer image that will install
the githook whenever a docker-compose up happens in the
repo. It will also install the hook whenever a `yarn`
occurs locally (as a postinstall hook).
This commit should also not fail things. For example
having unused variables is an ESLint error, but it isn't
autofixable. It will log the error, but will otherwise
continue. However, it will make this pretty with prettier
as well as fix any other autofixable ESLint errors.
closes CORE-2118
Test Plan:
- Run `yarn`
- Add some semicolons to something from the whitelist
- git add that file
- git commit and it will strip semicolons
- In a dockerized Canvas:
- docker-compose up
- Add semicolons to a file
- git add that file
- git commit, it will take forever (~60s)
- It should have stripped out semicolons
Change-Id: Id9198aa008808e898f29acb9ed64dd14ff843222
Reviewed-on: https://gerrit.instructure.com/171510
Tested-by: Jenkins
Reviewed-by: Brent Burgoyne <bburgoyne@instructure.com>
QA-Review: Brent Burgoyne <bburgoyne@instructure.com>
Product-Review: Brent Burgoyne <bburgoyne@instructure.com>
I guess since our team owns these we can opt them in now
Test plan:
* nothing should change
Change-Id: Ic7207e1033869ef60644c41bd5c80a3e8532a6dd
Reviewed-on: https://gerrit.instructure.com/171471
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
Tested-by: Jenkins
QA-Review: Ryan Shaw <ryan@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
This commit introduces a better more reasonable ESLint configuration.
It removes all concerns with styling in favor of using prettier to handle
those things.
closes CORE-2096
Test Plan:
- ESLint works
Change-Id: I50b90b7191b576bce4817d885d14f18c4c72d208
Reviewed-on: https://gerrit.instructure.com/170874
Tested-by: Jenkins
Reviewed-by: Steven Burnett <sburnett@instructure.com>
Reviewed-by: Brent Burgoyne <bburgoyne@instructure.com>
QA-Review: Clay Diffrient <cdiffrient@instructure.com>
Product-Review: Clay Diffrient <cdiffrient@instructure.com>
We do this often enough in our actual code that I don’t think it is
worth the noise we create complaining about it. I feel it conditions
people to ignore other stuff that does matter.
I also feel it is a valid thing to do and forcing people to use new
variable names just to appease this rule is not helpful.
Here’s the docs about why the rule exists:
https://eslint.org/docs/rules/no-param-reassign
What say ye’all? Can we axe this?
Change-Id: Ib9e3c54e5a02d3bbc892d875448d71ae87b5e6c1
Reviewed-on: https://gerrit.instructure.com/167894
Tested-by: Jenkins
Reviewed-by: Jeremy Stanley <jeremy@instructure.com>
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
this is only needed to protect yourself in hostile environments
where someone might do something like Object.prototype.foobar = “blah”
Because we control all the javascript that is executed on our page,
That won’t ever happen. And if it did, there are a million and a half
other places in our code that will blow up before your code blows up.
Besides, we already catch that ^ in `no-extend-native`
So complaining to you and making you handle this case is just not needed
To read the docs of why this rule exists go here:
https://eslint.org/docs/rules/guard-for-in
What say ye’all? can we axe this?
Change-Id: Ie942578e0b8a3cce8130412f866556da7b9e984c
Reviewed-on: https://gerrit.instructure.com/167896
Tested-by: Jenkins
QA-Review: Ryan Shaw <ryan@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
This rule is just there to protect against a pretty esoteric edge
case that we never do. So in effect, in our code, we never will run
into the case where you’ll need it
Read here for why you would need it:
https://eslint.org/docs/rules/no-prototype-builtins
What say ye’all can we axe this?
Change-Id: I4b9124dd457e843e8429eb701e1ddc77fc86c182
Reviewed-on: https://gerrit.instructure.com/167895
Tested-by: Jenkins
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
This makes it so everything in app/jsx/account_course_user_search
conforms to our style guide (prettier)
Change-Id: I797121e1002eb1a5bcd28f6a177db3b13ae029eb
Reviewed-on: https://gerrit.instructure.com/159747
Tested-by: Jenkins
Reviewed-by: Brent Burgoyne <bburgoyne@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Test Plan:
- Run ./node_modules/.bin/eslint "app/jsx/**/*.*" | grep 'jsx-a11y'
- Unfortunately you should see some errors :(
Change-Id: I5de6b15471393a7efe5a324b7b55347b7eb34311
Reviewed-on: https://gerrit.instructure.com/155918
Tested-by: Jenkins
Reviewed-by: Steven Burnett <sburnett@instructure.com>
Product-Review: Clay Diffrient <cdiffrient@instructure.com>
QA-Review: Clay Diffrient <cdiffrient@instructure.com>
These don’t objectively catch any bugs. They are just style preferences.
if people really want to do what they complain about, let’s assume they
know what they are doing and not bug them.
Eg: If they think it would be more readable to un-nest their ternary,
great. But if not, like if they are rendering a bunch of jsx lets not
make them make even more verbose and ugly code to not get “no-nested-
Ternaries” errors
And
<Foo ref={e => this.someRef = e} /> should be just fine. It doesn’t
help anything for people to make that more verbose
Change-Id: Ifa3461d8118038a20b42f568f682de3bae9f1f31
Reviewed-on: https://gerrit.instructure.com/153610
Tested-by: Jenkins
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Right now, eslint will tell you your imports should look like:
import bar from ‘compiled/foo/bar’
import {map} from ‘loadsh’
import baz from ‘../baz’
Once we get rid of all the “jsx/*” and “compiled/*” stuff from our
import paths and just use relative paths it will tell you to do:
import {map} from ‘loadsh’
import bar from ‘../../foo/bar’
import baz from ‘../baz’
So it doesn’t make sense to complain about this now and tell people
To put those compiled/foo paths up with the stuff from NPM and not with
the rest of the local stuff.
Note: it _is_ still encouraged to put all your imports in this order:
“nodeJS builtin (eg: import {readFile} from ‘fs’)”
"external (from node_modules)”
“local/relative (eg: ‘compiled/foo’ or ‘../bar’)”
There’s just not an eslint rule that can enforce that for us right now.
Change-Id: I83b0009882d525e9b999b8f9f5991361ce7981fa
Reviewed-on: https://gerrit.instructure.com/151102
Tested-by: Jenkins
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
if you set your editor to eslint --fix on save, which is already safe
be doing, it will also pass anything in this whitelist of dirs to
Opt-in to be prettier formatted too. So you don’t have to wait until
we prettier all the things later and can have consistency among your
now
Test plan:
* gergich should complain if something in app/jsx/permissions is not
Prettier formatted
Change-Id: I438d11f25b10ed58e53656e110aa0d1e7fed5660
Reviewed-on: https://gerrit.instructure.com/149583
Tested-by: Jenkins
Reviewed-by: Venk Natarajan <vnatarajan@instructure.com>
QA-Review: Steven Burnett <sburnett@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
these were all things gergich gives warnings for that everyone ignores
anyway. it is better to turn silly stuff off than to train people to
ignore gergich warnings
Change-Id: I0bc1f807c8c44c0fe6b180126cf8f6e9d14b1797
Reviewed-on: https://gerrit.instructure.com/147815
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
Tested-by: Jenkins
Product-Review: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Test plan:
* Run eslint —fix against a file that doesn’t have a copyright header
(Eg: eslint --fix app/coffeescripts/AssignmentDetailsDialog.js )
* it should add the copyright header for you
* run eslint on a file that already has a proper copyright header
* it should not duplicate it
Change-Id: I0bc05b6cc267fb74909f7a0fe0e93ecda1e2136e
Reviewed-on: https://gerrit.instructure.com/140757
Tested-by: Jenkins
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
This rule was configured to allow for conditional assignment
without forcing destructuring which requires the use of wrapping
parentheses and a lonely semicolon. This style was roundly
rejected after discussion.
Example:
let foo
;({foo} = bar)
Change-Id: I3cfe1be2ac18ce88481367830e2eb30effbc3340
Reviewed-on: https://gerrit.instructure.com/140916
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
Tested-by: Jenkins
Product-Review: Jeremy Neander <jneander@instructure.com>
QA-Review: Jeremy Neander <jneander@instructure.com>
This makes prettier errors eslint warnings. So when you
`eslint --fix some/file.js` it will format it with prettier
to try this out, apply this commit and run eslint against your files.
this change will make it so it runs it through prettier and forces
your code to look like prettier would output. But it doesn’t force
Prettier formatting to pass eslint since they are just warnings so if
you really want to format your code differently than prettier you can.
Change-Id: I4f1cdf4962173002a7dc22138be06cc66b842190
Reviewed-on: https://gerrit.instructure.com/131773
Tested-by: Jenkins
Reviewed-by: Jeremy Neander <jneander@instructure.com>
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
closes: CNVS-34337
* For now, you can only test things that don’t require any coffeescript
files
* Write your tests using es6 module syntax
* see app/jsx/shared/helpers/__tests__/parseLinkHeader.test.js for an
example
test plan:
* run `yarn run jest`
* you should see some output like:
PASS app/jsx/shared/helpers/__tests__/parseLinkHeader.test.js
PASS app/jsx/actAs/__tests__/ActAsModal.test.js
Test Suites: 2 passed, 2 total
Tests: 5 passed, 5 total
Snapshots: 1 passed, 1 total
Time: 3.072s
Ran all test suites.
* make sure it runs on jenkins build
Change-Id: Ia79cd8ce35dc863c4cc5ce7512998320c7cfdb84
Reviewed-on: https://gerrit.instructure.com/100685
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
Tested-by: Jenkins
Product-Review: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
…but still force `let foo=1, bar=2` to be:
let foo=1
let bar=2
I don’t actually find any value in enforcing this rule at all and think
we could ignore it altogether but rather than advocating we remove it
completely, this commit just makes it so you can combine uninitialized
variable declarations at the top.
I frequently see this in spec files where we do something like
let subject, result, whatever
//and then in a ‘beforeEach()’ we assign values to those
//and then in each test(..) they are used
What say ye all?
Test plan:
* look at follow-on commit to this that changes MessageStudentsSpec.js
* it should not have eslint errors about:
`error Split 'let' declarations into multiple statements`
Change-Id: I7aafac3a395bac221e9e9b3111a82d0af4671ddb
Reviewed-on: https://gerrit.instructure.com/134822
Tested-by: Jenkins
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
a common pattern to set a ref for a react element is:
<div ref={e => this.refName = e} />
But if you are using our eslint rules (currently set to
"no-return-assign": ["error", "always”]) it will force you to do:
<div ref={e => {this.refName = e}} />
And now, if you are using prettier, it will format that as:
<div ref={e => {
this.refName = e
}} />
That seems needlessly verbose. With this change you can now do:
<div ref={e => (this.refName = e)} />
And both eslint and prettier will be ok with that
Test plan:
* verify functions that return an assignment do what you want in eslint
Change-Id: I56abfdb21a7bf3f5b3eae8088f27749f41f2371c
Reviewed-on: https://gerrit.instructure.com/131788
Tested-by: Jenkins
Reviewed-by: Felix Milea-Ciobanu <fmileaciobanu@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
all of our code that uses AMD looks something like:
define([
‘jquery’,
‘underscore’
], ($, _) => {
$(doStuff)
…
It doesn’t make it any “better” to force people to remove
that blank line at the top of the callback. In fact, leaving it puts
it on par with what we already lint for to *enforce* a blank
line between es6 import or commonjs require() statements and the rest
of your code.
Change-Id: I3dd2051200f6bf458732940306cd044bf631b463
Reviewed-on: https://gerrit.instructure.com/112177
Tested-by: Jenkins
Reviewed-by: brian kirkby <bkirkby@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
…Because we have a ton of existing code that
Does things like `const $user_name = $('#user_name’)`
And it doesn’t make anything “better” to force people to
go around renaming all those. And, although with BEM / React
you probably wouldn’t write code like that in new stuff,
If the DOM ID really is #user_name, for greppability it arguably
is better to keep the js variable and the dom id the same.
(or at least it is not objectively worse than having one with
underscores and one in camelCase).
Change-Id: I042deb3a2bd612eaf74addb141015d288a7330fe
Reviewed-on: https://gerrit.instructure.com/112176
Tested-by: Jenkins
Reviewed-by: brian kirkby <bkirkby@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Closes: CNVS-36236
We now treat code in public/javascripts
exactly the same as app/jsx. Since we pass
it all to babel we can lint with the same rules
as we do app/jsx and you can use all the modern
syntax/features/babel goodness there. There is literally
no excuse to write old crapy javascript anymore.
Now we can start adding some commits that eslint -fix all the existing
stuff in public javscripts.
Test plan:
* if you run eslint public/javascripts/instructure.js it should
give you a bunch of warnings about converting things from var
to let/const and other stuff like that.
Change-Id: I5d400b958ae33f3f13f7f6a9c3505a4f7b5843db
Reviewed-on: https://gerrit.instructure.com/112146
Tested-by: Jenkins
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Test Plan:
- Breaking those rules will be okay or in the case of
named stuff it fails :)
Change-Id: I5d41ec8d12aee5a59b6f35da0a86ed1571950998
Reviewed-on: https://gerrit.instructure.com/100877
Tested-by: Jenkins
Reviewed-by: Ryan Shaw <ryan@instructure.com>
Product-Review: Clay Diffrient <cdiffrient@instructure.com>
QA-Review: Clay Diffrient <cdiffrient@instructure.com>
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
closes: CNVS-34124
These are it’s default rules:
"rules": {
"promise/always-return": "error",
"promise/no-return-wrap": "error",
"promise/param-names": "error",
"promise/catch-or-return": "error",
"promise/no-native": "off",
"promise/no-nesting": "warn",
"promise/no-promise-in-callback": "warn",
"promise/no-callback-in-promise": "warn",
"promise/avoid-new": "warn"
}
the only one I changed was set avoid-new to ignore
because I have noticed that there was a few times
where I really did need/want to make my own promise.
I am open to debate on what others think though
Test Plan:
* the eslint errors on your code that uses promises
should have the warnings you expect
Change-Id: I46e5f2d1d2ded2dba1e5aeb2ebcf303bcd4c5981
Reviewed-on: https://gerrit.instructure.com/98201
Tested-by: Jenkins
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Test Plan:
- Using ENV should not show as a problem anywhere
Change-Id: I4b37a95606755e21716df0dfb61f26a85e295fe9
Reviewed-on: https://gerrit.instructure.com/98198
Reviewed-by: Jeremy Neander <jneander@instructure.com>
Tested-by: Jenkins
Reviewed-by: Ryan Shaw <ryan@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Ed Schiebel