TS branch intial release #20

JohnMica · 2020-12-18T08:43:32Z

i am creating this PR agains the TS branch (that is what i understood from your latest replies Richard).

richardeschloss · 2020-12-19T02:36:10Z

.babelrc

@@ -0,0 +1,30 @@
+{


Just making a note: this file replaces babel.config.js

richardeschloss · 2020-12-19T02:37:41Z

.eslintrc.js

  rules: {
-    "space-before-function-paren": ["error", "never"],
+    // 'space-before-function-paren': ['error', 'never'],


This can probably be deleted altogether. the spacing conventions are probably set in one of the recommended linters above.

the idea would be that having at least 1 rule would make thing easier for users that would have different preferences when it comes to editor / linting rules

ok. I think we might also want the comma-dangle rule in here:

"comma-dangle": ["error", "never"],

richardeschloss · 2020-12-19T02:40:35Z

ava.config.cjs

+  return {
+    require: ["./test/helpers/ava.setup.js"],
+    ignoredByWatcher: ["!**/*.{js,vue}"],
+    babel: true,


As a personal preference, I find it useful to keep the files prop in there, even though it adds verbosity. I find it makes it much easier to test specific features in watch mode (but others have their preferences too). I've tried it both ways (the way you have here, and with files list; I can work faster with "test.only" and only specific files enabled)

richardeschloss · 2020-12-19T02:42:55Z

components/AfricaMap.vue

    }
  },
  computed: {
    chartOptions() {
      return {
        chart: {
-          map: 'africa'
+          map: 'africa',


I think this is fine for now, but I think eventually it'd make for much cleaner code if the chartOptions were stored somewhere else (in their own json), and then imported as needed, tweaking only the props of interest, such as the title text. But for now, this is fine.

that could work indeed and make things a bit simpler

richardeschloss · 2020-12-19T02:44:53Z

layouts/README.md

@@ -0,0 +1,7 @@
+# LAYOUTS


Let's delete this one for now.

even with this file not present, i still think it will be solved as a default layout by nuxt, no ?

i will remove this anyways

richardeschloss · 2020-12-19T02:46:48Z

nuxt.config.js

  head: {
-    title: process.env.npm_package_name || '',
+    title: "highcharts",


I think this should be reverted back to process.env.npm_package_name. This one of those minor things that's easy to miss as we go from project to project. This will be one less chore for us in the future.

richardeschloss · 2020-12-19T02:48:27Z

nuxt.config.js

+  // Modules for dev and build (recommended) (https://go.nuxtjs.dev/config-modules)
+  buildModules: [
+    // https://go.nuxtjs.dev/typescript
+    "@nuxt/typescript-build"


Do you have insight as to why the "require.context" error goes away in SSR-mode when using the typescript-build? I think it would be helpful to know not just here, but I think some folks ask about it on stackoverflow and other projects. (i.e., it's not a highcharts-specific issue, and others could probably be helped by the answer)

i am not 100% sure but

could be that using nuxt TS actually figures out the type of require in this particular function ?

Hmm. for some reason, I still encounter the warning, even with the ts build. I'll take some time to investigate the issue. I know the block of code causing it, I just need to troubleshoot ways to mute the warning (webpack/webpack#2675 might give some insight)

Ok, I was able to mute the warning and get this fixed. The key was to mock out the require.context.keys() method not require.context, which is what we want from our tests anyway.

richardeschloss · 2020-12-19T02:52:13Z

package.json

    "test": "cross-env NODE_ENV=test npm run test:specs:cov && npm run test:e2e:cov",
-    "test:specs": "cross-env TEST=specs ava --config specs.config.js",
+    "test:unit": "cross-env TEST=unit ava ./test/specs/**/*",


While this brief version is indeed prettier, it may come back to bite us later on when we want unit vs. e2e tests. I think it's easier to maintain when the script is instructed to use the specific ava config (the config for unit tests) and in that, it's files prop can specify the glob. That way, even if globs get checked in, but a specific feature is being worked on, it's easy to see in PRs if someone changed the glob. (personal preference, more my opinion)

so revert to the initial testing scripts?

I think so. That way any testing config changes take place in the respective ava config files. I find it's easier to manage the diffs that way. The test:unit and TEST=unit though can stay as-is because that's more common convention.

richardeschloss · 2020-12-19T02:53:15Z

package.json

    "test:e2e:cov": "cross-env TEST=e2e nyc --no-clean ava --timeout=10m --config e2e.config.js",
    "test:e2e:watch": "cross-env TEST=e2e ava --config e2e.config.js --color --watch"
  },
  "dependencies": {
-    "highcharts": "^8.1.0"
+    "@nuxt/typescript-runtime": "^2.0.0",


Does this alone fix the "require.context" issue when someone installs the library? Or will they also need @nuxt/typescript-build?

even now using the Nuxt TS would actually add @nuxt/typescript-build to the deps and in the 'how to' it is mentioned as well.
so if users want the TS they do need the package (until v3 is released and we will need to figure out what and how)

richardeschloss · 2020-12-19T02:54:08Z

pages/index/_chart.vue

@@ -21,7 +21,7 @@ export default {
    StockChart,
    MapChart,
    QuickMods,
-    SunburstChart
-  }
+    SunburstChart,


Hmm. do the recommended lint conventions have dangling commas?

it could be that my Vetur options might have this by default ?

Ok, I put the eslint recommendation above (for the eslint file)

richardeschloss · 2020-12-19T02:55:45Z

test/e2e/index.spec.js

@@ -0,0 +1,29 @@
+import { resolve } from 'path'


I think this may be extraneous for now. It may be a useful blueprint for e2e testing, but I think it can be deleted (or safely stashed away with git stash)

richardeschloss · 2020-12-19T02:58:50Z

test/helpers/ava.setup.js

@@ -0,0 +1,15 @@
+if (process.env.TEST === 'unit') {


I'd prefer to keep ava.setup.js as-is for now. It also requires browser-env which I think was needed for some tests. Having this file in helpers may be a good place for it though.

i will revert the file to its original setup

richardeschloss · 2020-12-19T02:59:22Z

test/specs/Logo.spec.js

@@ -0,0 +1,8 @@
+import test from 'ava'


Extraneous. Can be removed.

richardeschloss · 2020-12-19T03:00:32Z

Also, can you please run npm run test:specs:cov to ensure all tests pass still with 100% coverage? I am pretty confident they will

JohnMica · 2020-12-21T10:16:41Z

Also, can you please run npm run test:specs:cov to ensure all tests pass still with 100% coverage? I am pretty confident they will

this seems to be causing an error

[Error - 10:15:37 AM] Error: .eslintrc.js » @nuxtjs/eslint-config-typescript » @nuxtjs/eslint-config:
	Environment key "jest/globals" is unknown```
looking into this now

richardeschloss · 2020-12-21T17:57:43Z

For that error, I think you might need eslint-plugin-jest ?

richardeschloss · 2020-12-22T04:46:03Z

Branch feat/ts now contains the nuxt-ts deps. Closing this PR. Thank you for your help.

TS branch intial release

262ea4f

richardeschloss reviewed Dec 19, 2020

View reviewed changes

.babelrc

@@ -0,0 +1,30 @@

{

Copy link

Owner

richardeschloss Dec 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just making a note: this file replaces babel.config.js

richardeschloss reviewed Dec 19, 2020

View reviewed changes

richardeschloss closed this Dec 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TS branch intial release #20

TS branch intial release #20

JohnMica commented Dec 18, 2020

richardeschloss Dec 19, 2020

richardeschloss Dec 19, 2020

JohnMica Dec 21, 2020

richardeschloss Dec 21, 2020

richardeschloss Dec 19, 2020

richardeschloss Dec 19, 2020

JohnMica Dec 21, 2020

richardeschloss Dec 19, 2020

JohnMica Dec 21, 2020

richardeschloss Dec 19, 2020

JohnMica Dec 21, 2020

richardeschloss Dec 19, 2020

JohnMica Dec 21, 2020

richardeschloss Dec 21, 2020

richardeschloss Dec 21, 2020

richardeschloss Dec 19, 2020 •

edited

Loading

JohnMica Dec 21, 2020

richardeschloss Dec 21, 2020

richardeschloss Dec 19, 2020

JohnMica Dec 21, 2020

richardeschloss Dec 19, 2020 •

edited

Loading

JohnMica Dec 21, 2020

richardeschloss Dec 21, 2020

richardeschloss Dec 19, 2020

richardeschloss Dec 19, 2020

JohnMica Dec 21, 2020

richardeschloss Dec 19, 2020

JohnMica Dec 21, 2020

richardeschloss commented Dec 19, 2020 •

edited

Loading

JohnMica commented Dec 21, 2020

richardeschloss commented Dec 21, 2020

richardeschloss commented Dec 22, 2020

TS branch intial release #20

TS branch intial release #20

Conversation

JohnMica commented Dec 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardeschloss Dec 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardeschloss Dec 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardeschloss commented Dec 19, 2020 • edited Loading

JohnMica commented Dec 21, 2020

richardeschloss commented Dec 21, 2020

richardeschloss commented Dec 22, 2020

richardeschloss Dec 19, 2020 •

edited

Loading

richardeschloss Dec 19, 2020 •

edited

Loading

richardeschloss commented Dec 19, 2020 •

edited

Loading