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

Adopt type=module #202

Merged
merged 14 commits into from Jun 5, 2021
Merged

Adopt type=module #202

merged 14 commits into from Jun 5, 2021

Conversation

Fil
Copy link
Member

@Fil Fil commented Apr 26, 2021

follow changes in d3-format:

  • type=module
  • add exports
  • remove zip
  • license: ISC
  • update dependencies

TODO:

follow changes in d3-format:
* type=module
* add exports
* remove zip
* license: ISC
* update dependencies

TODO: the tests on *global* fail.
@Fil Fil requested a review from mbostock April 26, 2021 13:04
@mbostock
Copy link
Member

Do the tests on global fail because the code is now in strict mode, so global is undefined? Or is it globalThis?

@Fil
Copy link
Member Author

Fil commented Apr 26, 2021

From what I see global is OK (when the tests run), but this is undefined. And eslint complains that global is undef.

@mbostock
Copy link
Member

I think since the code is now in strict mode we expect this to be undefined.

@Fil
Copy link
Member Author

Fil commented Apr 26, 2021

another (last?) issue, is how to load the barley test data (json file).

what I do here gives an eslint error
https://github.com/d3/d3-array/pull/202/files#diff-d1c9a415ec5b6150109e5d874042dab86178f271a9b3114e383a6058e25dbbdeR4
5:31 error Parsing error: Unexpected token import

@aulonm
Copy link

aulonm commented Apr 26, 2021

deepEqual from assert is deprecated, you should use deepStrictEqual instead (ref the documentation)

Same thing with equal, maybe use strictEqual

Comment on lines +5 to +8
"ecmaVersion": 8
},
"env": {
"es6": true,
Copy link

Choose a reason for hiding this comment

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

This needs to be at least version 11 for the import.meta.url to work.

I think we need to decide if we want to go with es6 or jump all the way up to es2020 (which version 11 is part of). It is possible to keep env es6 and use ecmaVersion 11, but not sure if that is the way to go here. (I'd rather just jump all the way up to es2020, or es2021)

Ref this discussion here

Copy link
Member Author

Choose a reason for hiding this comment

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

I just need to find a way to load the json file that makes everyone happy (ie tests and eslint)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, let’s not use require here. Let’s use fs.readFileSync or similar.

Copy link

@aulonm aulonm Apr 26, 2021

Choose a reason for hiding this comment

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

You can also rename the file to a .js instead of .json, and inside of the js-file you can do a default export [...]. This way its possible to import the js-file into the tests and they will run as usual. Then you wont need fs.readFileSync.

Not sure what your usual coding practices have been with d3js, so I'll let you guys decide whats best :)

Copy link
Member Author

Choose a reason for hiding this comment

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

now let's see if it passes CI :)

@mbostock mbostock marked this pull request as ready for review June 5, 2021 15:26
@mbostock mbostock merged commit edd0492 into main Jun 5, 2021
@mbostock mbostock deleted the fil/type-module branch June 5, 2021 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants