Notes & ideas for improvments

[ahmedelgabri]

First, this is a very nice project & well executed. I was thinking already about doing something similar, so since its done I'd rather contribute to this one.

So I had a quick look & here are some of my findings & general observations.

Ideas

• Update dependencies, there are lots of outdated packages
• I'd recommend to at least lock dependencies versions & stop using ranges. This won't lock dependinces of the depencieces, if this is required we can look into using npm shrinkrap which is a bit annoying & has its own issues

  Project wide, worklfow & tools issues

• .stylelintrc format is messed up & .eslintrc should be JSON a couple of globals are missing quotes.
• Why pass query for babel inside Webpack while already have .babelrc file?

• why modules: false?

  presets: ['react', ['es2015', {'modules': false}], 'stage-0', 'react-hmre'],

• any reason for ["system-import-transformer", {"modules": "common"}]?
• why provide jQuery as global? and why these globals? Webpack can allow for shimming global dependencies to be used as normal imports or requires. Some of them are not even used at all.
• In general the project doesn't follow linter rules which can lead to some subtle bugs & inconsistent style

I'd like to fix all the issues mentioned, unless anyone else disagrees. And I'd like to discuss the two ideas for improvements before start looking into other things.

[ahmedre]

cc @mmahalwy

[mmahalwy]

Salam @ahmedelgabri

Firstly, thanks for taking the time to look through the project, and I am super excited to see you contributing code! Most of the concerns or lack of the project being 100% is time - we all do this on the side, so time is limited, so more contributors = better off!

To address the questions:

Update dependencies, there are lots of outdated packages

Agreed. That needs to happen

I'd recommend to at least lock dependencies versions & stop using ranges. This won't lock dependinces of the depencieces, if this is required we can look into using npm shrinkrap which is a bit annoying & has its own issues

I personally hate working with the shrinkwrap, it only causes more challenges and pain than it solves. But I hear that uber shrinkwrap is a better alternative. We can try it?

.stylelintrc format is messed up & .eslintrc should be JSON a couple of globals are missing quotes.

Yep, needs fixing.

Why pass query for babel inside Webpack while already have .babelrc file?

We are an isomorphic app, thus, the server and the client code are shared, but they don't necessarily have the same configs for each. That's why we specify them in the webpack config because they differ. To give you an example, system-import-transformer is supported natively by webpack but not babel, so we need to install a package for babel on the server. As for the modules: false is because modules is natively supported by webpack 2, but not on the server :)

why provide jQuery as global? and why these globals? Webpack can allow for shimming global dependencies to be used as normal imports or requires. Some of them are not even used at all.

We should remove what we don't need but jquery is global because initially I did all the tooltips and fancy bootstrap components with react-bootstrap but caused a significant decrease in rendering speed as now you have a tooltip for every word on the page. Using jquery with bootstrap solved the rendering speed problem. It's not perfect. I know.

In general the project doesn't follow linter rules which can lead to some subtle bugs & inconsistent style

We do run tests on listing and so right now it's passing eslint - what rules specifically?

[ahmedelgabri]

Salam @mmahalwy :)

I personally hate working with the shrinkwrap, it only causes more challenges and pain than it solves. But I hear that uber shrinkwrap is a better alternative. We can try it?

Can't agree more. Let's first lock the normal versions & then see how we can deal with the whole tree. I was looking into uber shrinkwrap for work and the problem that it doesn't work with npm@3 & the project requires node@6.x which comes with npm@3 by default.

There are other options like shrinkpack or npm-lockdown I didn't use any of them yet, but I lean more towards npm-lockdown.

---

We are an isomorphic app, thus, the server and the client code are shared, but they don't necessarily have the same configs for each. That's why we specify them in the webpack config because they differ. To give you an example, system-import-transformer is supported natively by webpack but not babel, so we need to install a package for babel on the server. As for the modules: false is because modules is natively supported by webpack 2, but not on the server :)

Ok this is a bit clear for being isomorphic, but there is another way to make this less confusing maybe rename .babelrc to .babelrc.js if babel supports this or move the contents of .babelrc inside package.json under a babel key then require this file into webpack config and create the webpack config based on this. This will keep everything in one place & removes the duplication. Here is a quick example:

inside package.json

...
  "babel": {
    "presets": ["react", "es2015", "stage-0"],
    "plugins": [
      "transform-runtime",
      "add-module-exports",
      "transform-decorators-legacy",
      "transform-react-display-name",
      ["system-import-transformer", {"modules": "common"}]
    ],
    "env": {
      "development": {
        "plugins": [
          "typecheck"
        ]
      },
      "production": {
        "plugins": [
          "transform-react-inline-elements",
          "transform-react-constant-elements"
        ]
      }
    }
  },
...

inside dev.config.js for example

const babelConfig = require('../package.json').babel;
...
{
  test: /\.js$/,
  exclude: /node_modules/,
  loaders: [
    {
      loader: 'babel',
      query: {
        plugins: babelConfig.plugins.filter(plugin => !plugin.isArray()),
        presets:  babelConfig.presets.concat([['es2015', {modules: false}], 'react-hmre']),
        cacheDirectory: true
      }
    }
  ]
},
...

Then you will have a base babel config, which is the one coming from package.json & you extend it if/when needed.

As for system-import-transformer why do you use it for? I have a general idea about System.import() & how it works but I want to understand the use case here.

For webpack, that's interesting because now after you said that the project is using webpack2 I think the configs are not right, because webpack2 configs should export a function accepting an options object here is some info from webpack author not an object as in webpack1. Maybe they decided to support the object as backward compatibility thing.

---

We should remove what we don't need but jquery is global because initially I did all the tooltips and fancy bootstrap components with react-bootstrap but caused a significant decrease in rendering speed as now you have a tooltip for every word on the page. Using jquery with bootstrap solved the rendering speed problem. It's not perfect. I know.

Bootstrap is always an issue. With tooltips you mean the ones on the ayat right? This one can easily be done with CSS/HTML alone without any JavaScript at all. If these are the tooltips you mean I'll be happy to fix them. Also, if you have a plugin/dependency that requires jQuery as global maybe use the imports-loader instead of creating global variables.

---

We do run tests on listing and so right now it's passing eslint - what rules specifically?

1. lint commands are not the same npm run test:ci:lint & npm run test:dev:lint
2. linting is only happening on src not the whole project, which kind of defies the purpose of having a linter since you want all you code to be linted. Not just a section of it. And here is why
   • fs is not used
   • duplicate path requires here & here
   • duplicate node keys here & here
   • and there is more in other files too...

That's just in one file outside ./src & it is an important one. I'd highly recommend installing a eslint linter plugin for your editor to get a direct feedback while working on files.

---

Sorry for the long reply!

[mmahalwy]

@ahmedelgabri no worries! Those are all really good thoughts mashallah. Here is what I suggest: let's tackle the low hanging fruits first then work our way to the more decision-making challenges. For example, let's get rid of jQuery and have the tooltips via CSS and convert the dropdowns to react-bootstrap that will significantly decrease the size of our assets. Then fix the eslint problems.

re system.import: https://github.com/thgreasi/babel-plugin-system-import-transformer because webpack allows usage of System.import but node does not know what System is and thus breaks. We use System.import to do webpack chunking, so the user doesn't need to download parts of the app that they may never or hardly visit (contact page, search page, etc.). We have to be conscious of the size of our js assets because we have visitors from all over the world.

[ahmedelgabri]

@mmahalwy Agree, will start looking into this this week inshallah.

For System.import I know also that webpack uses require.ensure() to do code splitting.

[mmahalwy]

That is one way, but the new way is System.import which resolves a promise. See https://gist.github.com/sokra/27b24881210b56bbaff7#code-splitting-with-es6

[mmahalwy]

Let me know if you need any help or get stuck. @sabeurthabti or I can help out :)

[ahmedelgabri]

Sure, will do :)

[atalebagha]

Here are some ideas I thought of that are non-technical:

• Select/view different Qira2at/narrations (Arabic)
• Hover over dictionary for the Arabic language
• Tafaseer (it would be like a translation, appear below the ayah)
• Notepad (this is big, as the database will grow)
• Tajweed Color code (like in the Dar Al-Ma'arifah print)

[mmahalwy]

@atalebagha added to that, i started working on a user auth. Just need to dpeloy it

[mmahalwy]

@ahmedelgabri @atalebagha let's break these into individual issues?

[ahmedelgabri]

Salam @mmahalwy, sorry I have been very busy last couple of months with work & personal stuff. Will try to have a look at the weekend inshallah

[mmahalwy]

okay perfect! Thank you