-
Notifications
You must be signed in to change notification settings - Fork 41
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
Feature/esm jest #35
Feature/esm jest #35
Conversation
This is now supported by node (out of the box)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felixge Can you let me know your thoughts? It would also be nice to upgrade to github actions at some point but I do not have time for that atm.
@@ -0,0 +1,5 @@ | |||
{ | |||
"presets": [ | |||
"@babel/preset-env" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is currently required for jest but I expect soon we can drop babel for tests.
(function testBasic() { | ||
var trace = get(); | ||
|
||
//expect(trace[0].getFunction()).toBe(testBasic); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felixge I'm not sure why but getFunction always returns undefined/null here
(function testWrapper() { | ||
(function testBelowFn() { | ||
var trace = get(testBelowFn); | ||
//expect(trace[0].getFunction()).toBe(testWrapper); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felixge I'm not sure why but getFunction always returns undefined/null here
2: 'Object.asyncJestTest', | ||
4: 'new Promise' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels weird, but we are parsing what is passed to us. It probably could be better
compare('getTypeName', { | ||
7: null | ||
}); | ||
compare('getMethodName', { | ||
2: 'asyncJestTest' | ||
}); | ||
compare('getLineNumber', { | ||
0: 88, | ||
1: 92 | ||
}); | ||
compare('getColumnNumber', { | ||
0: 13 | ||
}); | ||
compare('isNative'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These overrides seem brittle depending on node version?
return lines | ||
.map(function(line) { | ||
if (line.match(/^\s*[-]{4,}$/)) { | ||
return self._createParsedCallSite({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the whole reason I migrated to esm. When this script is used in esm (you can import cjs) this self reference was globalThis and caused runtime issues.
@@ -0,0 +1,51 @@ | |||
import { get } from "../index.js"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved and renamed these tests, not sure why it thinks it was a delete and new instead of a git mv
@felixge I'd like your thoughts before merging this unless you are fine with these changes since they are breaking (dropping cjs). I'm currently blocked on my esm project. |
@niemyjski this LGTM. I don't have time for an in-depth review. My only thought is that perhaps it's time to give this a major (v1) version number to make it clear it's a breaking changing and then keep doing the semver thing going forward. |
Sounds good! I will push this as |
Upgraded tests to jest and dropped cjs for esm. Everyone is converting their libraries to esm which supports importing cjs but not the other way around. I was running into issues importing and consuming parse from esm due to a
self
reference.