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.

Sign up for GitHub

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

Jump to bottom

Chore: Fix/remove tools (fixes #471) #476

Merged
merged 3 commits into from Apr 21, 2021
Merged

Chore: Fix/remove tools (fixes #471) #476

merged 3 commits into from Apr 21, 2021

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented Apr 16, 2021

  1. I removed a bunch of scripts in the tools/ directory that were left over from the initial Esprima migration and were no longer used.
  2. For tools that were heavily reliant on require(), I opted to just rename the file to .cjs.

@nzakas nzakas requested a review from a team April 16, 2021 17:55
mdjermanovic
mdjermanovic requested changes Apr 19, 2021
View reviewed changes
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

We should also update both create-test.js and update-tests.js to produce ESM content in *.result.js files, because the tests will import those files and all *.js files are now ESM. In particular, we should replace module.exports = with export default here and here.

@nzakas
Copy link
Member Author

nzakas commented Apr 19, 2021

Should be set now. Also found that the scripts didn't work with the bigint tests, so fixed that too.

@mdjermanovic mdjermanovic linked an issue Apr 19, 2021 that may be closed by this pull request
Fix broken scripts in tools/ and convert to esm #471
Closed
mdjermanovic
mdjermanovic approved these changes Apr 20, 2021
View reviewed changes
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM!

@mdjermanovic
Copy link
Member

There's a merge conflict in package.json.

@nzakas
Copy link
Member Author

nzakas commented Apr 21, 2021

Conflict fixed.

@nzakas nzakas merged commit 651e204 into main Apr 21, 2021
@nzakas nzakas deleted the issue471 branch April 21, 2021 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@btmills btmills btmills approved these changes

@mdjermanovic mdjermanovic mdjermanovic approved these changes

Assignees
No one assigned
Labels
accepted chore
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix broken scripts in tools/ and convert to esm
3 participants
@nzakas @mdjermanovic @btmills