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.

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

Linting and Component Refactoring #314

Merged
merged 1 commit into from Sep 25, 2018

Conversation

Projects
None yet
2 participants
@theduckylittle
Copy link
Member

theduckylittle commented Jul 16, 2018

  • Added new stricter linting rules.
  • Cleaned up code based on those rules.
  • Refactoring components to provide clearer division of labor (State, Rendering, Logic are now all more clearly divided).
  • Ensured that const is used appropriately for better browser hinting.
  • Replacing for(... in ...) with proper for(... i < ii ...) loops as the for .. in loops are transformed poorly and performance is bad.

@theduckylittle theduckylittle changed the title Linting and Component Refactoring WIP - Linting and Component Refactoring Jul 16, 2018

@theduckylittle theduckylittle force-pushed the theduckylittle:component-refactor branch 4 times, most recently from 246af92 to c9b31a2 Jul 16, 2018

*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* of this software and associated documentation files (the 'Software'), to deal

This comment has been minimized.

@klassenjs

klassenjs Jul 16, 2018

Member

Why do some of these use ' and others use " ?

This comment has been minimized.

@theduckylittle

theduckylittle Jul 16, 2018

Author Member

%s/"/'/g has caught the license text.

let active = [];
for(let ms in mapSources) {
if(isMapSourceActive(map_sources[ms])) {
const active = [];

This comment has been minimized.

@klassenjs

klassenjs Jul 16, 2018

Member

How did this work before?

This comment has been minimized.

@theduckylittle

theduckylittle Jul 16, 2018

Author Member

I'm not sure it did! Or it worked due to a scope issue with var

@klassenjs

This comment has been minimized.

Copy link
Member

klassenjs commented Jul 16, 2018

npm install is failing for me. Any ideas?

npm ERR! write after end
npm ERR! write after end
npm ERR! write after end
npm ERR! write after end
npm ERR! write after end
npm ERR! write after end
@theduckylittle

This comment has been minimized.

Copy link
Member Author

theduckylittle commented Jul 16, 2018

What version of node / npm? I'm not having any issues.

@klassenjs

This comment has been minimized.

Copy link
Member

klassenjs commented Jul 17, 2018

v8.9.4 and v5.7.1. I'll try updating.

@klassenjs

This comment has been minimized.

Copy link
Member

klassenjs commented Jul 17, 2018

grunt build fails

Running "eslint:target" (eslint) task
Warning: Cannot find module 'eslint-config-react-app'
Referenced from: /home/jimk/Public/GeoMoose/gm3/eslint.config.js Use --force to continue.

Aborted due to warnings.
@theduckylittle

This comment has been minimized.

Copy link
Member Author

theduckylittle commented Jul 17, 2018

Some stuff is missing from the package.json... I'll need to play catch up with it

@klassenjs

This comment has been minimized.

Copy link
Member

klassenjs commented Jul 17, 2018

Also noticed npm audit has some suggestions.

@klassenjs

This comment has been minimized.

Copy link
Member

klassenjs commented Sep 20, 2018

Did you know canvas made it into package.json?

@klassenjs

This comment has been minimized.

Copy link
Member

klassenjs commented Sep 20, 2018

I am getting eslint errors:

$ grunt build
Running "lintless:all" (lintless) task
Linting...OK

Running "eslint:target" (eslint) task

./gm3/src/index.js
  60:1  warning  Unexpected var, use let or const instead  no-var
  75:1  warning  Unexpected var, use let or const instead  no-var

./gm3/src/services/findme.js
  60:9   warning  Unexpected var, use let or const instead    no-var
  61:9   warning  Unexpected var, use let or const instead    no-var
  62:9   warning  Unexpected var, use let or const instead    no-var
  62:13  warning  'coord' is assigned a value but never used  no-unused-vars
  65:9   warning  Unexpected var, use let or const instead    no-var
  77:9   warning  Unexpected var, use let or const instead    no-var
  77:27  error    'gm3' is not defined                        no-undef
  84:9   warning  Unexpected var, use let or const instead    no-var
  85:9   warning  Unexpected var, use let or const instead    no-var

./gm3/src/services/geocode-bing.js
  56:9   warning  Unexpected var, use let or const instead  no-var
  68:9   warning  Unexpected var, use let or const instead  no-var
  70:9   warning  Unexpected var, use let or const instead  no-var
  71:9   error    'gm3' is not defined                      no-undef
  96:17  warning  Unexpected var, use let or const instead  no-var
  97:17  warning  Unexpected var, use let or const instead  no-var
  98:21  warning  Unexpected var, use let or const instead  no-var
  99:21  warning  Unexpected var, use let or const instead  no-var

./gm3/src/services/geocode-osm.js
  59:9   warning  Unexpected var, use let or const instead  no-var
  71:9   warning  Unexpected var, use let or const instead  no-var
  73:9   warning  Unexpected var, use let or const instead  no-var
  74:9   warning  Unexpected var, use let or const instead  no-var
  75:9   error    'gm3' is not defined                      no-undef
  84:17  warning  Unexpected var, use let or const instead  no-var
  85:21  warning  Unexpected var, use let or const instead  no-var
  86:21  warning  Unexpected var, use let or const instead  no-var
  98:28  error    'gm3' is not defined                      no-undef

./gm3/src/services/identify.js
  71:9   warning  Unexpected var, use let or const instead  no-var
  88:9   warning  Unexpected var, use let or const instead  no-var
  90:13  warning  Unexpected var, use let or const instead  no-var
  92:13  warning  Unexpected var, use let or const instead  no-var

./gm3/src/services/search.js
   68:9   warning  Unexpected var, use let or const instead  no-var
   69:13  warning  Unexpected var, use let or const instead  no-var
  113:9   warning  Unexpected var, use let or const instead  no-var
  115:13  warning  Unexpected var, use let or const instead  no-var
  117:13  warning  Unexpected var, use let or const instead  no-var
  158:9   warning  Unexpected var, use let or const instead  no-var
  159:13  warning  Unexpected var, use let or const instead  no-var
  160:13  warning  Unexpected var, use let or const instead  no-var

./gm3/src/services/select.js
  105:13  warning  Unexpected var, use let or const instead  no-var
  106:13  warning  Unexpected var, use let or const instead  no-var
  107:13  warning  Unexpected var, use let or const instead  no-var
  114:13  warning  Unexpected var, use let or const instead  no-var
  116:13  warning  Unexpected var, use let or const instead  no-var
  133:9   warning  Unexpected var, use let or const instead  no-var
  137:13  warning  Unexpected var, use let or const instead  no-var
  139:13  warning  Unexpected var, use let or const instead  no-var

./gm3/src/tasks/grunt-less-linter.js
  33:7   warning  'fs' is assigned a value but never used         no-unused-vars
  45:17  warning  'chr' is never reassigned. Use 'const' instead  prefer-const
  77:9   warning  Unexpected var, use let or const instead        no-var

✖ 51 problems (4 errors, 47 warnings)
  0 errors, 45 warnings potentially fixable with the `--fix` option.

Warning: Task "eslint:target" failed. Use --force to continue.

Aborted due to warnings.
@theduckylittle

This comment has been minimized.

Copy link
Member Author

theduckylittle commented Sep 20, 2018

Try npm run build.

@klassenjs

This comment has been minimized.

Copy link
Member

klassenjs commented Sep 20, 2018

$ npm run build
npm ERR! missing script: build

npm ERR! A complete log of this run can be found in:
npm ERR!     .../.cache/npm/_logs/2018-09-20T16_32_00_109Z-debug.log
@theduckylittle

This comment has been minimized.

Copy link
Member Author

theduckylittle commented Sep 20, 2018

Serves me right... I'm working on fixing the linting issue right now. I see that I have not moved the build script into package.json yet.

@theduckylittle

This comment has been minimized.

Copy link
Member Author

theduckylittle commented Sep 20, 2018

I realized I forgot to lint the tests as well.

@klassenjs

This comment has been minimized.

Copy link
Member

klassenjs commented Sep 20, 2018

It passes the tests and builds now. It seems to work, but haven't tested extensively.

@theduckylittle theduckylittle changed the title WIP - Linting and Component Refactoring Linting and Component Refactoring Sep 20, 2018

@klassenjs
Copy link
Member

klassenjs left a comment

Why?

@theduckylittle

This comment has been minimized.

Copy link
Member Author

theduckylittle commented Sep 20, 2018

Trigger travis.

@theduckylittle theduckylittle force-pushed the theduckylittle:component-refactor branch from c1ff11a to 15b5afe Sep 21, 2018

@klassenjs

This comment has been minimized.

Copy link
Member

klassenjs commented Sep 21, 2018

One test failed:

 FAIL  tests/gm3/components/toolbar.test.js
  ● Test suite failed to run

    Cannot find module './button' from 'index.js'

    
      
      at Resolver.resolveModule (node_modules/jest-resolve/build/index.js:169:17)
      at Object.<anonymous> (src/gm3/components/toolbar/index.js:223:41)

@klassenjs

This comment has been minimized.

Copy link
Member

klassenjs commented Sep 21, 2018

Looks like the build fails on missing button also.

@theduckylittle theduckylittle force-pushed the theduckylittle:component-refactor branch from 15b5afe to 6813afd Sep 21, 2018

@theduckylittle

This comment has been minimized.

Copy link
Member Author

theduckylittle commented Sep 21, 2018

Rookie mistake! Forgot to add in two new files.

@klassenjs

This comment has been minimized.

Copy link
Member

klassenjs commented Sep 21, 2018

The "circle with check mark" in the catalog/visible tabs is confusing to me. Looks like a radio-button, acts like a check-box.

The Measure tool doesn't work. Super Tab is blank.

@klassenjs

This comment has been minimized.

Copy link
Member

klassenjs commented Sep 21, 2018

Otherwise looks good.

@klassenjs

This comment has been minimized.

Copy link
Member

klassenjs commented Sep 22, 2018

Looks good to me.

Lint Clean-up and Refactor
- Added stricter linting rules.
- Conformed to new linting rules.
- Dropped JSX convention
- Punting more state management back to redux.
- Collapsing the logic for rendering a catalog to something
   more functional (as in coding style).
- Adding Provider to make the tools and layers less
   silly when it comes to working with the state tree
   and dispatching actions.
- Modal is now prop and state controlled
- Layer tools are now connected as appropriate.
- Refactored logic, state, and rendering code to be
   more clearly divided.
- Clean up of Coordinate display logic
- Improved toolbar coverage.
- Converted catalog checkboxes to icons.

@theduckylittle theduckylittle force-pushed the theduckylittle:component-refactor branch from 0412d65 to 20b0bcb Sep 25, 2018

@theduckylittle theduckylittle merged commit eff1264 into geomoose:master Sep 25, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.