shopify / javascript Goto Github PK

View Code? Open in Web Editor NEW

253.0 455.0 38.0 656 KB

The home for all things JavaScript at Shopify.

License: MIT License

javascript's People

Contributors

Stargazers

Watchers

javascript's Issues

Fix `mocha-context-to-closure` error when mixing setup/this/CallExpression

esify spits out this error:

undefined does not match field "name": string of type Identifier
  test/theme-editor-next-javascripts/functional/design_mode/shopify_api_next_test.coffee

The problem is in this setup code:

    for key, event of DesignMode.API.EVENTS
      @[key.toLowerCase()] = @spy()
      document.removeEventListener(event, @[key.toLowerCase()])

Things are fine if:

remove-useless-return-from-test is disabled
@[key.toLowerCase()] lines are changed to foo[key.toLowerCase()]
The toLowerCase() call is removed

Minimal failing test input:

suite('DesignMode.ShopifyApi', function() {
  setup(function() {
    for (let [key, event] of foo) {
      this[key.toLowerCase()] = bar;
    }
  });
});

Fix missing switch/case returns in `channel-dashboard`

channel-dashboard.coffee contains this switch statement:

  _dateFieldForDays: (days) ->
    switch
      when days == 0 then 'hour'
      when days < 90 then 'day'
      when days < 365 then 'week'
      else 'month'

This is translated to JS that's missing returns:

  _dateFieldForDays(days) {
    return (() => {
      switch (false) {
      case !(days === 0):
        'hour';
        break;
      case !(days < 90):
        'day';
        break;
      case !(days < 365):
        'week';
        break;
      default:
        'month';
      }
    })();

Clean up `threw` assertions

We still have assert.okay(foo.threw, 'error message')-style assertions. These should probably be converted to assert.fail('error message';), or the original CoffeeScript should be altered to not use this pattern.

Class property definitions should not disappear

class Shopify.AutocompleteV2
  LIST_CLASS = '.js-autocomplete-suggestions'
  PIXELS_FROM_BOTTOM_TO_INFINITE_SCROLL = 500
  ERROR_DISPLAY_TIME = 5000

export default class AutocompleteV2 {
  constructor(node, renderer, options = {}) {

Add better Node Babel config

Provide a better experience for Mocha using expect

By default, no-unused-expressions and the newline for chained call rules cause a lot of issues, so they should be turned off. Would probably make sense to set the sinon and expect/ assert globals as well.

Automate cleanup of "Return statement should not contain assignment" error

The CoffeeScript conversion will generate 1,000+ blocks of code like:

   return window.location = location;

Unfortunately, our lint config will flag all of these as errors.

The prosaic fix is to just transform the above into separate assign+return lines. Is there any way to make this more intelligent? There are specific cases like return console.log(...) that may be useful to consider.

Simplify ?= operator transform output

resource_browser.coffee contains:

    parentState.__id ?= _.uniqueId()

Which becomes the rather verbose:

    if (parentState.__id != null) {
      parentState.__id;
    } else {
      parentState.__id = _.uniqueId();
    }

Lint rule to prevent literals as the "actual" part of an assertion

Chai assertions use the first argument as the "actual" value. To guarantee consistent error reporting, the following code should be flagged by a lint rule:

// BAD.
assert.equal(1, foo);
assert.equal(2, foo);
assert.equal("foo", bar);
assert.equal({foo: "bar"}, qux);
assert.equal([1, 2, 3], foo);

strictEqual, deepEqual, ok, notStrictEqual, notOk, etc should be checked, too.

Clarify preference for conditions

Prefer:

if (a) {}/ if (!a) {}

over:

if (Boolean(a)) {}/ if (!Boolean(a)) {}/ if (!!a) {}

Multiple `flash.coffee` files break global-reference-to-import transform

If legacy + refactored versions of a file exist, the global-reference-to-import-transform bails.

Found multiple definitions for Shopify.Flash: 
admin/lib/flash.coffee:9:1:Shopify.Flash = Flash =
theme_editor/lib/flash.coffee:9:1:Shopify.Flash = Flash =
theme_editor_next/lib/flash.coffee:9:1:Shopify.Flash = Flash =

Per @lemonmade, we can probably ignore the theme_editor files and just notify the team that it's something they'll have to address before they run their code through esify.

Prefer early returns to full-function conditionals.

Add a shopify-node Babel preset and ESLint config

For node projects, it would be nice to have babel transpiling or ESLint complaining about features that are not available in the version of Node/ npm we are trying to use wherever possible.

Remove returns from anonymous `addEventListener` handlers

Vanilla and jQuery handlers treat return values very differently. Removing (completely meaningless) explicit return values from addEventListener handlers should discourage conflating of techniques.

Example:

document.addEventListener('input', event => {
  if (event.target.type === 'hidden') {
    event.preventDefault();
    return event.stopPropagation();
  }
}, true);

Add rules for good Promise/ async patterns

Good starting point for custom rules: https://www.npmjs.com/package/eslint-plugin-promise

Remove hanging `return void 0` from if alternates

The decaf process generates 54 functions that end with a pointless else block:

foo = () => {
  if (bar != null) {
    return bar(true);
  } else { // REMOVE THIS ELSE.
    return void 0;
  }
}

These should be removed.

Look into adding eslint-plugin-babel

Typo: "difficuly"

https://github.com/Shopify/javascript#6.2

difficuly = difficult

Verify shared instance properties -> class properties

Noted in the second example of this issue: https://github.com/juliankrispel/decaf/issues/127.

Currently, we are translating this:

class Foo
  bar: 123

to this:

class Foo {
  bar = 123;
}

But these have different semantics: the CoffeeScript one goes on the prototype (shared between all instances) where the JavaScript one goes on the instance.

Look into adding eslint-plugin-lodash

Discourage alternatives to `assert.fail`

Code like assert.ok(false, 'error message') should be flagged as an error. If possible, add a fix option to translate to assert.fail('error message').

Grammer: Too many "yet"s

https://github.com/Shopify/javascript#7.6

"which does not yet support these yet" should be either:

"which does not yet support these"
"which does not support these yet"

Fix conditional assignments

The operation.done callback contains this CoffeeScript assignment:

      if @nextPageParams?
        @title = "Load next #{@LARGE_PAGE_SIZE} products"

The transformed JS is missing a this.title assignment:

      if (this.nextPageParams != null) {
        this.nextPageParams;
      }

Decaf: investigate soaks that may dereference `void 0`

admin/lib/element-queries.js contains a line that'll attempt to access (void 0).width if its condition fails:

var parentWidth = (((ref = node.parentNode) != null ? ref.getBoundingClientRect() : void 0)).width;

Original CoffeeScript:

parentWidth = node.parentNode?.getBoundingClientRect().width

Add a transform to match the `sinon-prefer-meaningful-assertions` linting rule

Non-meaningful sinon assertions end up being a big chunk of the linting errors on our output files. A transform to get rid of them would be awesome, and the logic is largely already figured out in the lint rule.

cc/ @GoodForOneFare

Fix late super constructor calls in shopify/shopify

super should be the first constructor line in all of the following files:

admin/lib/autocomplete_v2/autocomplete_v2.coffee
admin/lib/autocomplete_v2/remote_multi_select_autocomplete.coffee
admin/lib/autocomplete_v2/renderers/lazy_loaded_basic_renderer.coffee
admin/lib/autocomplete_v2/shipping_zone_autocomplete.coffee
admin/lib/autocomplete_v2/single_select_remote_source_autocomplete.coffee
admin/lib/autocomplete_v2/tags_autocomplete.coffee
admin/lib/confirm.coffee
admin/lib/embedded_app_hosts/admin_embedded_app_host.coffee
admin/lib/embedded_app_hosts/pos_embedded_app_host.coffee
admin/lib/flash.coffee
admin/modules/articles/article_form.coffee
admin/modules/capital/accept_offer_form.coffee
admin/modules/collections/collection_form.coffee
admin/modules/dashboard/channel_dashboard.coffee
admin/modules/dashboard/channel_pos_dashboard.coffee
admin/modules/dashboard/kpi_multi_time.coffee
admin/modules/discounts/discount_form.coffee
admin/modules/domains/domain_form.coffee
admin/modules/domains/remote_domain_form.coffee
admin/modules/draft_orders/customer_autocomplete.coffee
admin/modules/filter_resource_picker.coffee
admin/modules/financial_reports/financial_summary_report.coffee
admin/modules/financial_reports/payment_summary_report.coffee
admin/modules/financial_reports/pending_sales_summary_report.coffee
admin/modules/financial_reports/product_sales_summary.coffee
admin/modules/financial_reports/showReport.coffee
admin/modules/force_dirty_state_form.coffee
admin/modules/form_with_credit_card.coffee
admin/modules/form_with_poller.coffee
admin/modules/fulfillments/fulfillments_new_form.coffee
admin/modules/home/custom_feed_card.coffee
admin/modules/home/insights/insights_massive_spike_from_new_referrer_card.coffee
admin/modules/home/sidebar/home_channels_summary_report.coffee
admin/modules/home/sidebar/home_sidebar_report.coffee
admin/modules/inventory_transfers/inventory_transfer_form.coffee
admin/modules/inventory_transfers/supplier_autocomplete.coffee
admin/modules/link_lists/link_list_form.coffee
admin/modules/orders/customer_address_form.coffee
admin/modules/payments/activation_forms/au.coffee
admin/modules/payments/activation_forms/ca.coffee
admin/modules/payments/activation_forms/gb.coffee
admin/modules/payments/activation_forms/payments_activation_form.coffee
admin/modules/payments/activation_forms/payments_managed_accounts_activation_form.coffee
admin/modules/payments/activation_forms/us.coffee
admin/modules/product_resource_browser.coffee
admin/modules/product_variants/update_inventory_modal_form.coffee
admin/modules/products/product_create_form.coffee
admin/modules/products/product_options_autocomplete.coffee
admin/modules/products/product_options_edit.coffee
admin/modules/products/product_variant_options.coffee
admin/modules/products/variant_operations.coffee
admin/modules/rte/insert_image_modal.coffee
admin/modules/shipping/country_autocomplete.coffee
admin/modules/shipping/province_autocomplete.coffee
admin/modules/shipping/shipping_rate_form.coffee
admin/modules/shipping/shipping_zone_form.coffee
admin/modules/shipping/test_carrier_rates_modal.coffee
admin/modules/shipping_labels/pay_outstanding_invoices_form.coffee
admin/modules/taggable.coffee
admin/modules/tax_code_column.coffee
admin/modules/taxes/american_states_autocomplete.coffee
admin/modules/ui_country_autocomplete.coffee
admin/modules/users/user_form.coffee
unit/financial_summary/financial_summary_report_test.coffee

Why prefer js file extension over jsx?

Surely the file is not valid JS so it would be prudent to use jsx.

Document assertion transforms

And remember to update the CHANGELOG.

Decaf: investigate mangled block regex

While reviewing the string cleanup PR, I'm seeing what looks like a broken regex conversion. It may not be an issue, but I'm trying to concentrate on other stuff right now.

design_model/live-colors.coffee

  commentRegex: (key) ->
    /// \/\*\!setting\.[\"\s]*#{Strings.escapeRegexpString(key)}[\"\s]*\{\*\/([^\}]*)\/\*\}\*\/ ///g

Before string cleanup, this translates to

  return RegExp(('\\/\\\\*\\\\!setting\\\\.[\\"\\\\s]*' + (Strings.escapeRegexpString(key)) + '[\\"\\\\s]*\\\\{\\\\*\\\\/([^\\\\}]*)\\\\/\\\\*\\\\}\\\\*\\\\/'), 'g');

After string cleanup, it translates to

return RegExp(('\\/\\\\*\\\\!setting\\\\.["\\\\s]*' + (Strings.escapeRegexpString(key)) + '["\\\\s]*\\\\{\\\\*\\\\/([^\\\\}]*)\\\\/\\\\*\\\\}\\\\*\\\\/'), 'g');

A file shouldn't try to import itself

admin/lib/money.js tries to import Money from admin/lib/money``:

'expose Shopify.Money';
import _ from 'lodash';
import Number from 'admin/lib/number';
import CurrencyDB from 'admin/currency_db.coffee';
import Money from 'admin/lib/money';
const MONEY_FORMAT_REGEX = /(\{\{\s*(\w+)\s*\}\})/;
const COMMON_DELIMITERS = '\',.';

export default class Money {

Also affected: admin/lib/time, admin/modules/domains/domain-form.

Run `eslint --fix` as part of `esify`

ESLint can fix a bunch of minor formatting things automatically. It would be nice to run it after all the decaf/ codemods have run on a file, and it should be easy enough to do given that we know eslint is installed locally given that this is running in the Shopify repo. Depending on whether we can easily access that eslint, it would also be a reasonable alternative to have eslint as a global peer dependency, or as a dependency and using it programatically.

Arguments to function calls sometimes look weird

Here's the output of esify on app/assets/javascripts/admin/modules/apple_pass_settings.coffee:

Ideally, we would like the constructor definition to look like this:

class ApplePassSettings {
  constructor({
    defaultLogoSrc,
    defaultStripSrc,
    currentLogoSrc,
    currentStripSrc,
  }) {}
}

Although it should be on a single line if that line would be short enough t one readable. At a minimum, we should try to get rid of the trailing comma, not sure we have the Babel transform to kill those.

Clean up `no-cond-assign` errors

An if condition with an assignment should be split into two lines:

// Before
if (img = files[0]) {
  foo();
}

// After
img = files[0];
if (img) {
  foo();
}

Other examples from shopify/shopify code:

if (href = button.href) {
if (message = $node.text()) {
if (previousInstance = previousContext[key]) {

Add Shopify to dustinspecker/awesome-eslint?

Would it be alright to add Shopify to this?

https://github.com/dustinspecker/awesome-eslint

Namely the plugin section.

Decaf: investigate array comprehension issue

In app/assets/javascripts/admin/modules/background_queue_poller.coffee, the following:

Gets transformed to this through esify:

This code is weird and bad anyways, so maybe we can just kill it, but we should check to make sure that other comprehensions are not also getting removed like this.

Use es-like lodash names

Lodash's docs show that the following _ methods can be safely renamed to their non-aliased versions.

Old name	Standard name
_.first	_.head
_.each	_.forEach
_.eachRight	_.forEachRight
_.entries	_.toPairs
_.entriesIn	_.toPairsIn
_.extend	_.assignIn
_.extendWith	_.assignInWith

Fix soaked conditional assigments

admin_embedded_app_host.coffee contains a couple of conditional assignments like:

pagination.previous = Shopify.EmbeddedAppButtons.build(pagination.previous) if pagination?.previous?

This gets rewritten to a complex if that doesn't assign to pagination.previous:

    if (((typeof pagination !== 'undefined' && pagination !== null ? pagination.previous : void 0)) != null) {
      if (typeof pagination !== 'undefined' && pagination !== null) {
        pagination.previous;
      } else {
        void 0;
      }
    }

Make `global-reference-to-import` faster

Based on a facile profile of transforms, global-reference-to-import accounts for ~60% of esify's runtime.

Most likely, this is caused by continually searching the codebase with ag/ack. Adding a cache may significantly speed this up.

Timing based on a full transform of shopify/shopify/**/*.coffee, with ag.

Transform	Time (minutes)
shopify-codemod/global-reference-to-import	11.65
js-codemod/no-vars	1.54
shopify-codemod/mocha-context-to-closure	0.35
shopify-codemod/global-identifier-to-import	0.34
shopify-codemod/coffeescript-soak-to-condition	0.32
shopify-codemod/function-to-arrow	0.32
shopify-codemod/remove-useless-return-from-test	0.31
shopify-codemod/ternary-statement-to-if-statement	0.31
js-codemod/unquote-properties	0.27
shopify-codemod/coffeescript-range-output-to-helper	0.27
shopify-codemod/conditional-assign-to-if-statement	0.27
shopify-codemod/remove-addeventlistener-returns	0.27
shopify-codemod/constant-function-expression-to-statement	0.26
js-codemod/template-literals	0.25
shopify-codemod/strip-template-literal-parenthesis	0.25
shopify-codemod/remove-trailing-else-undefined-return	0.25
js-codemod/object-shorthand	0.25
shopify-codemod/remove-empty-returns	0.24
shopify-codemod/mocha-context-to-global-reference	0.17
shopify-codemod/global-assignment-to-default-export	0.16
js-codemod/arrow-function	0.14
shopify-codemod/assert/call-count-equals-to-assert-called	0.14
shopify-codemod/assert/called-method-to-assert-called	0.13
shopify-codemod/assert/move-literals-to-expected-argument	0.12
shopify-codemod/assert/called-with-methods-to-assert-called-with	0.12
shopify-codemod/assert/negated-assert-ok-to-assert-not-ok	0.12
shopify-codemod/assert/assert-to-assert-ok	0.12
shopify-codemod/assert/assert-false-to-assert-fail	0.12
shopify-codemod/assert/called-equals-boolean-to-assert-called	0.12
shopify-codemod/assert/equality-comparisons-to-assertions	0.12
shopify-codemod/assert/falsy-called-method-to-assert-not-called	0.11

global-reference-to-import doesn't ignore transformed files

Once a file is transformed, global-reference-to-import flags Coffee+JavaScript exports as conflicting definitions.

e.g.,

Error: Found multiple definitions for Shopify.ElementQueries:
admin/lib/element_queries.coffee:136:1:Shopify.ElementQueries =
admin/lib/element-queries.js:1:1:'expose Shopify.ElementQueries';

Look into adding/ integrating other ESLint plugins

Some good ones that might be relevant to us:

Missing Definition for some jsx-a11y rules

I have a meteor (1.3.2.4) project with react where I added eslint-plugin-shopify.

All my files complain of missing jsx-a11y rules, on the first line (import React from 'react')

  1:1  error    Definition for rule 'jsx-a11y/img-uses-alt' was not found                    jsx-a11y/img-uses-alt
  1:1  error    Definition for rule 'jsx-a11y/label-uses-for' was not found                  jsx-a11y/label-uses-for
  1:1  error    Definition for rule 'jsx-a11y/mouse-events-map-to-key-events' was not found  jsx-a11y/mouse-events-map-to-key-events
  1:1  error    Definition for rule 'jsx-a11y/no-hash-href' was not found                    jsx-a11y/no-hash-href
  1:1  warning  Definition for rule 'jsx-a11y/redundant-alt' was not found                   jsx-a11y/redundant-alt
  1:1  error    Definition for rule 'jsx-a11y/valid-aria-role' was not found                 jsx-a11y/valid-aria-role

My package.json is:

{
  "name": "board",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1",
    "lint": "eslint . --max-warnings 0"
  },
  "author": "",
  "license": "ISC",
  "dependencies": {
    "moment": "^2.13.0",
    "moment-timezone": "^0.5.3",
    "react": "^0.14.8",
    "react-dom": "^0.14.8",
    "react-redux": "^4.4.1",
    "react-router": "^2.0.1",
    "redux": "^3.5.2",
    "redux-thunk": "^2.0.1"
  },
  "eslintConfig": {
    "extends": [
      "plugin:shopify/react"
    ],
    "env": {
      "meteor": true,
      "node": true
    },
    "rules": {
      "no-warning-comments": "off",
      "no-console": "off",
    },
    "plugins": [
      "jsx-a11y"
    ]
  },
  "devDependencies": {
    "eslint": "^2.9.0",
    "eslint-plugin-babel": "^3.2.0",
    "eslint-plugin-import": "^1.6.1",
    "eslint-plugin-jsx-a11y": "^1.0.4",
    "eslint-plugin-shopify": "^11.1.2",
    "eslint-plugin-sort-class-members": "^1.0.1"
  }
}

So far, I have removed this lines on "rules" manually now, but I guess there is a problem with the way the shopify plugin uses jsx-a11y.

    "rules": {
      "no-warning-comments": "off",
      "no-console": "off",
      "jsx-a11y/img-uses-alt": "off",
      "jsx-a11y/label-uses-for": "off",
      "jsx-a11y/mouse-events-map-to-key-events": "off",
      "jsx-a11y/no-hash-href": "off",
      "jsx-a11y/redundant-alt": "off",
      "jsx-a11y/valid-aria-role": "off"
    },

Esify should not delete sprocket directives

Manifest files are being ignored by Esify.

Steps to reproduce:

esify app/assets/javascripts/admin/admin_vendors.coffee
See app/assets/javascripts/admin/admin_vendors.js

admin_vendors.js is empty because admin_vendors.coffee is commented. Decaf treats it as an empty file and thus isn't compiling it.

Main culprits

app/assets/javascripts/admin/admin_vendors.coffee
app/assets/javascripts/admin/merchant_checkout.coffee

cc @lemonmade

Look into adding eslint-plugin-flowtype

Remove redundant returns

If a return with no value is the last line of a function, just remove it.

Examples:

  function whenFinished() {
    unhighlight(target, startTime);
    listeners.remove();
    return;
  }

  $document.on('click', 'a.disabled, a.btn-disabled', event => {
    event.preventDefault();
    return;
  });

  constructor(sourceArray, templates) {
    this.sourceArray = sourceArray;
    this.templates = templates;
    return;
  }

Investigate `undefined` dereferences generated by decaf

In admin/modules/fraft_orders/draft-builder.js, I'm seeing a method call on undefined:

      if (undefined.lineItemDiscounts[index] != null) {
        item.applied_discount = undefined.lineItemDiscounts[index].serializedDiscount();
      }

Original CS:

      item.applied_discount = @lineItemDiscounts[index]?.serializedDiscount()

admin/modules/template-editor.js

        key: (ref = undefined.currentTab) != null ? ref.key : void 0,

Convert default export objects to named exports

export default {
  foo: 'bar',
  baz: qux,
};

would be better written as:

export const foo = 'bar';
export const baz = qux;

CoffeeScript compilation of `"""` causes incorrect escaping

This is probably an issue with our decaf fork, but putting it here for posterity. The following CoffeeScript:

FIXTURE = """
  <div id="outer" style="height: #{CONTAINER_HEIGHT}px; overflow: scroll">
    <div id="inner" style="height: #{CONTENT_HEIGHT}px"></div>
  </div>
"""

Gets compiled to this through esify:

const FIXTURE = (`<div id=\\"outer\\" style=\\"height: ${CONTAINER_HEIGHT}px; overflow: scroll\\">\\n  <div id=\\"inner\\" style=\\"height: ${CONTENT_HEIGHT}px\\"></div>\\n</div>`);

Remove returns from named event handler functions

If a function is defined and only ever used as an addEventListener/removeEventListener parameter, it's safe to remove any redundant returns from the function.

As a reference point, here's a very simplified version of the transformed design_mode_next/buttons.js:

function onMouseOver(event) {
  renderSectionButtons(section);
  return; // REMOVE THIS.
}

function enable() {
   document.addEventListener('mouseover', onMouseOver, true);
}

Spurious error on jsx-key in tests

I'm migrating the script editor to use to shared eslint settings and I hit a small issue. We have a bunch of unit tests that have the following structure.

let renderer = ReactTestUtils.createRenderer();
renderer.render(<Foo a="1" b="2" />);
let output = renderer.getRenderOutput();
expect(output.props.children).toEqual([
  <Bar a="1" />,
  <Baz b="2" />,
]);

eslint flags it as an error for the rule jsx-key, but it does not apply here afaik. I couldn't find a way to disable this rule for test files only, so I just completely disabled it.

I guess the point of this issue is just to ask if there's a workaround. If so can it be included in the shared config or documented for others.

cc: @lemonmade

Provide a better initial experience for CSS modules users

Right now, basically all of the import rules will fail on imported CSS files.

shopify / javascript Goto Github PK

javascript's People

Contributors

Stargazers

Watchers

Forkers

javascript's Issues

Recommend Projects

Recommend Topics

Recommend Org