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: move to native ESM during dev + jsdoc TS #20

Merged
merged 9 commits into from
Apr 8, 2021
Merged

chore: move to native ESM during dev + jsdoc TS #20

merged 9 commits into from
Apr 8, 2021

Conversation

kentcdodds
Copy link
Owner

@kentcdodds kentcdodds commented Apr 7, 2021 •
edited
Loading

This is the solution I hate the least. Running into this on the tests though:

$ node --experimental-vm-modules node_modules/.bin/jest
(node:84738) ExperimentalWarning: VM Modules is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
 FAIL  src/__tests__/index.js
  ● Test suite failed to run

    ReferenceError: global is not defined

      at Object.<anonymous> (node_modules/expect/build/index.js:3:14)
      at TestScheduler.scheduleTests (node_modules/@jest/core/build/TestScheduler.js:337:13)
      at runJest (node_modules/@jest/core/build/runJest.js:377:19)
      at _run10000 (node_modules/@jest/core/build/cli/index.js:406:7)
      at runCLI (node_modules/@jest/core/build/cli/index.js:261:3)

Test Suites: 1 failed, 1 total
Tests:       0 total
Snapshots:   0 total
Time:        0.516 s
Ran all test suites.

Reproduction is in this PR: #20

I poked around a bit but have no idea why this could be happening. Didn't find anyone else with the same problem either 😅

Oh, and I only got that when I upgraded to 27.0.0-next.7. The current version of jest I'm on is 26.6.3 and with that I get:

 FAIL  src/__tests__/index.js
  ● Test suite failed to run

    Jest encountered an unexpected token

    This usually means that you are trying to import a file which Jest cannot parse, e.g. it's not plain JavaScript.

    By default, if Jest sees a Babel config, it will use that to transform your files, ignoring "node_modules".

    Here's what you can do:
     • If you are trying to use ECMAScript Modules, see https://jestjs.io/docs/en/ecmascript-modules for how to enable it.
     • To have some of your "node_modules" files transformed, you can specify a custom "transformIgnorePatterns" in your config.
     • If you need a custom transformation specify a "transform" option in your config.
     • If you simply want to mock your non-JS modules (e.g. binary assets) you can stub them out with the "moduleNameMapper" config option.

    You'll find more details and examples of these config options in the docs:
    https://jestjs.io/docs/en/configuration.html

    Details:

    /Users/kentcdodds/code/mdx-bundler/node_modules/remark-mdx-frontmatter/node_modules/js-yaml/dist/js-yaml.mjs:3831
    export default jsYaml;
    ^^^^^^

    SyntaxError: Unexpected token 'export'

      at Runtime.createScriptFromCode (node_modules/jest-runtime/build/index.js:1350:14)
      at Object.<anonymous> (node_modules/remark-mdx-frontmatter/dist/index.js:6:19)

Test Suites: 1 failed, 1 total
Tests:       0 total
Snapshots:   0 total
Time:        1.691 s
Ran all test suites.

I haven't had a chance to dig on that yet.

Not sure what that's all about... Everything else seems to work pretty well. I've tried to comment around the code that is a little weird.

@kentcdodds kentcdodds mentioned this pull request Apr 7, 2021
Open
21 tasks
kentcdodds
kentcdodds commented Apr 7, 2021
View reviewed changes
.huskyrc.js
@@ -1 +0,0 @@
module.exports = require('kcd-scripts/husky')
Copy link
Owner Author

Choose a reason for hiding this comment

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

This must be an ESM module with our configuration now. But I'm planning on updating the version of husky so I just removed it for now.

@Pomax
Copy link

Pomax commented Apr 7, 2021 •
edited
Loading

Just a note that your tests seem to import leftpad, which is probably unnecessary given that vanilla JS has that functionality built in as String.padStart, obviating a reasonable chunk of that test file.

@kentcdodds
Copy link
Owner Author

@Pomax, thanks. It's just for the test. We don't need the functionality, we're just testing a feature of mdx-bundler :)

@kentcdodds
Copy link
Owner Author

I think this should work. Would love feedback from folks on this.

@kentcdodds kentcdodds requested a review from Arcath April 8, 2021 03:40
Arcath
Arcath reviewed Apr 8, 2021
View reviewed changes
src/__tests__/index.js Outdated
`)

assert.snapshot(error.message, `Build failed with 1 error:
__mdx_bundler_fake_dir__/demo.tsx:1:7: error: [inMemory] Could not resolve "./blah-blah" from "./demo.tsx"`)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test is failing on Windows. Changing to:

assert.snapshot(error.message, `Build failed with 1 error:
__mdx_bundler_fake_dir__/demo.tsx:1:7: error: [inMemory] Could not resolve "./blah-blah" from ".${path.sep}demo.tsx"`)

does fix the problem, however I suspect any automatic updates to the snapshot will break it again in the future.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Fixed 👍

uvu doesn't auto-update snapshots, but I switched it to .equal anyway. Thanks!

Arcath
Arcath previously approved these changes Apr 8, 2021
View reviewed changes
@kentcdodds kentcdodds merged commit 5fe9485 into main Apr 8, 2021
@kentcdodds kentcdodds deleted the pr/fix-everything branch April 8, 2021 14:46
@github-actions
Copy link

github-actions bot commented Apr 8, 2021

🎉 This PR is included in version 3.1.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Arcath Arcath Arcath left review comments

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kentcdodds @Pomax @Arcath