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

Update enumify to 2.x #448

Merged
merged 2 commits into from
Apr 24, 2020
Merged

Update enumify to 2.x #448

merged 2 commits into from
Apr 24, 2020

Conversation

kinow
Copy link
Member

@kinow kinow commented Apr 23, 2020

This is a small change with no associated Issue.

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Already covered by existing tests.
  • No change log entry required (why? e.g. invisible to users).
  • No documentation update required.

@kinow kinow added this to the 0.2 milestone Apr 23, 2020
@kinow kinow self-assigned this Apr 23, 2020
@kinow
Copy link
Member Author

kinow commented Apr 23, 2020

This was a small change that was complicated by a series of factors. First, the library was updated to TypeScript, and using static fields in classes, which needs a Babel plug-in to work (rauschma/enumify#21). However, adding that plug-in broke the instrumentation we have in place for tests.

That's because Istanbul was configured to run before Babel. So the Babel plug-ins had no effect (webpack-contrib/istanbul-instrumenter-loader#89).

Fixing the first problem was easy, just a matter of installing and loading the plug-in.

The second one took me longer than expected, but also taught me more about Babel, Istanbul, and our current setup with Vue 2.

In order to fix the second problem, of Istanbul not taking into consideration Babel plug-ins, first I found that the latest releases (3.x and 4.) of istanbul-lib-instrument used by istanbul-instrumenter-loader (we use this loader to instrument for coverage) works well with Babel plug-ins.

So I remembered reading about resolutions in the Yarn docs. This is similar to how transitive dependencies can be overridden in Maven. We basically tell Yarn to ignore the dependency used by the library, and instead trust us and use the version we give it.

Once that's done, everything works.

This same procedure would have to be done if we ever needed to use other Babel plug-ins, so I think it should be OK to add it now.

@@ -1,5 +1,8 @@
module.exports = {
presets: [
'@vue/app'
],
plugins: [
['@babel/plugin-proposal-class-properties', { loose: true }]
Copy link
Member Author

Choose a reason for hiding this comment

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

So that we support static fields in classes with ES6. (loose so that it is less strict with how we define the static fields)

@@ -7,7 +7,7 @@
"build": "vue-cli-service build",
"build:report": "vue-cli-service build --report",
"build:watch": "vue-cli-service build --watch --mode development",
"checkpoint" : "./src/services/mock/generate",
"checkpoint": "./src/services/mock/generate",
Copy link
Member Author

Choose a reason for hiding this comment

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

Done by yarn add...

@@ -89,6 +90,9 @@
"vuetify-loader": "^1.3.0",
"webpack": "^4.41.0"
},
"resolutions": {
"istanbul-instrumenter-loader/**/istanbul-lib-instrument": "4.0.1"
},
Copy link
Member Author

Choose a reason for hiding this comment

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

Overriding the dependency of the loader, as the new Istanbul instrument library loads Babel plug-ins. Alas it seems like a release with the new version might take a while over there.

@oliver-sanders
Copy link
Member

😭

TypeError: Super expression must either be null or a function

@kinow
Copy link
Member Author

kinow commented Apr 23, 2020

sob

TypeError: Super expression must either be null or a function

Did you get that running yarn run serve? Did you run yarn install too? (Forgot to mention that would be necessary)

Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Forgot NODE_ENV again!!!!!!

class TaskState extends Enumify {
// NOTE: the order of the enum values is important to calculate the group states in the UI. Items at
// the top of the list have preference over the items below in the algorithm.
static SUBMIT_FAILED = new TaskState('SUBMIT_FAILED')
Copy link
Member

Choose a reason for hiding this comment

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

class TaskState has attribute which are TaskState instances, very meta!

@hjoliver hjoliver merged commit aaf8607 into cylc:master Apr 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants