-
-
Notifications
You must be signed in to change notification settings - Fork 366
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
Require Node.js 12 and drop UMD build #315
Conversation
test/browser.js
Outdated
type: 'module', | ||
content: ` | ||
import ky from './index.js'; | ||
window.ky = ky; |
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.
Any ideas why this doesn't work? I'm still getting:
browser › headers are preserved when input is a Request and there are searchParams in the options
Rejected promise returned by test. Reason:
Error {
message: `Evaluation failed: TypeError: window.ky is not a function␊
at __puppeteer_evaluation_script__:6:17`,
}
› __puppeteer_evaluation_script__:6:17
› ExecutionContext._evaluateInternal (node_modules/puppeteer/lib/cjs/puppeteer/common/ExecutionContext.js:217:19)
› ExecutionContext.evaluate (node_modules/puppeteer/lib/cjs/puppeteer/common/ExecutionContext.js:106:16)
› file://test/browser.js:331:2
› withPage (file://test/helpers/with-page.js:7:3)
in the browser tests.
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.
An educated guess... I think what's happening is that the browser is trying to load /index.js
but there's no such route on the server
in the tests. We didn't need a route prior to this PR because Puppeteer itself (on the Node.js side) was reading the file at path
from the filesystem and injecting it inline as a string in a script tag.
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 managed to solve it. Really weird that the import
call didn't throw though.
@@ -2,27 +2,6 @@ | |||
|
|||
const globals = {}; | |||
|
|||
const getGlobal = property => { |
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 could break some edge cases, including the scenario from #203 and #202. Basically, I've seen cases where some of the globals we rely on were actually defined on different objects (i.e. not true globals), particularly in mocked test environments.
That said, we're less reliant on these globals since the last time I had any issues and now that Node.js actually has globalThis
and I can do globalThis.window = globalThis
... maybe it won't be a problem? I'm not entirely sure. I'm 👍 on simplifying this, but let's definitely mark it as a breaking change.
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.
Let's try and see. I would prefer for us to not have to have lots of workarounds for weird use-cases.
b0d076c
to
8caec2b
Compare
I'm skipping the browser tests on CI for now. They always succeed locally, but randomly fail with |
I'm sorry, but full esm means, that consumers should rewrite their code to esm too, otherwise it would be error |
@rifler You need to use |
I have decided to target Node.js 12 and drop the UMD build. Node.js 10 will be obsolete in a few months anyway.
This lets us go full ESM, which reduces the chances of problems as some bundlers cannot handle
exports
in package.json.