Skip to content
This repository has been archived by the owner on Jul 30, 2018. It is now read-only.

Converting to intern4 #110

Merged
merged 4 commits into from
Sep 7, 2017
Merged

Converting to intern4 #110

merged 4 commits into from
Sep 7, 2017

Conversation

rorticus
Copy link
Contributor

Type: feature

The following has been addressed in the PR:

  • There is a related issue
  • All code matches the style guide
  • Unit or Functional tests are included in the PR

Description:

Updating to work with intern 4. Depends on dojo/grunt-dojo2#146.

package.json Outdated
@@ -31,7 +31,7 @@
"@types/node": "~6.0.49",
"grunt": "~1.0.1",
"grunt-dojo2": "latest",
"intern": "^3.4.1",
"intern": "next",
Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend using a specific version as the API in 'next' may change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pinned this to 4.0.0-alpha.13

@@ -0,0 +1,14 @@
export * from './intern';
Copy link
Member

Choose a reason for hiding this comment

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

Note that Intern no longer uses JS modules for config. The configuration is now a JSON file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These files are turned into JSON using a convoluted process in grunt-dojo2, you can see that here: dojo/grunt-dojo2#146

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I wondered if that might be case. Unfortunate that it means you can't run the tests without grunt-dojo2, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well the reason I left them as js files was because we load them and extend them. If that is not a pattern we should be using w/ intern4, what's the way we should be doing that? Is it ideal to just have the different json files that we keep in sync?

Copy link
Member

Choose a reason for hiding this comment

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

A config file can extend another, similar to how tsconfig.json files work.

@@ -57,7 +56,7 @@ registerSuite({
},

'delete': {
before() {
before(this: any) {
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't need to provide a type for this in a test (and this method doesn't even appear to use this).

Copy link
Member

@jason0x43 jason0x43 left a comment

Choose a reason for hiding this comment

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

The main issue I see is that Intern doesn't use JS config files any more (maybe grunt-dojo2 is still doing something with them). The this typings in the Map tests also seem like they shouldn't be necessary.

import pollUntil = require('intern/dojo/node!leadfoot/helpers/pollUntil');

registerSuite({
registerSuite('asyncAwait', {
'Async/Await with Bluebird'(this: any) {
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't need the explicit this here.

@rorticus
Copy link
Contributor Author

rorticus commented Sep 6, 2017

ci failures are from the docs..

Copy link
Member

@kitsonk kitsonk left a comment

Choose a reason for hiding this comment

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

Coolness!

@rorticus rorticus merged commit 708b6b1 into dojo:master Sep 7, 2017
@rorticus rorticus deleted the intern4 branch September 7, 2017 11:13
@dylans dylans added this to the 2017.09 milestone Sep 7, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants