-
Notifications
You must be signed in to change notification settings - Fork 803
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
Create webpack 4 plugin to emulate fancy webpack builds in wrangler 2 #759
Conversation
🦋 Changeset detectedLatest commit: 84893e1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
"dependencies": { | ||
"execa": "^6.1.0", | ||
"rimraf": "^3.0.2", | ||
"wrangler": "../wrangler" |
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 don't think you need this because of the wonders of npm workspaces.
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.
👀
Exciting! |
A wrangler prerelease is available for testing. You can install this latest build in your project with: npm install --save-dev https://prerelease-registry.developers.workers.dev/runs/2168849381/npm-package-wrangler-759 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.developers.workers.dev/prs/759/npm-package-wrangler-759 Or you can use npx https://prerelease-registry.developers.workers.dev/runs/2168849381/npm-package-wrangler-759 dev path/to/script.js |
660be71
to
2cc2f6d
Compare
This needs to be gone through with a fine-toothed comb.
i need it for my tests
sounds like a problem for the cass of tomorrow. i also had to make some changes to the mocks for upload requests because the filename isn't always `index.js`, i dunno if that's a big deal or not. Also there's probably a better way to check if they're doing sourcemaps, because it looks like webpack generates a default thing.
it should function basically the same https://stackoverflow.com/a/57505954/6894799
and also change expected script name to be a parameter instead of "index.js"
this is a squash of a bunch of commits so i'm not sure _exactly_ what's all been done, but it basically has to do with normalizing paths on windows, stripping out sparkle emojis in snapshots (they're not printed on windows), and caching wrangler 1 as an artifact in CI so that it doesn't have to be redownloaded all the time
f404245
to
ee29d17
Compare
Feel free to cut corners including just not running tests on windows (as long as you tested the actual thing on windows and it works as expected there?). I don't want this dragging you down. |
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.
As I am tired, it might be worth getting a second opinion from @threepointone and @JacobMGEvans. But it looks totally awesome!
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 PR is pretty fkin incredible. Approving with some questions.
fs.readdirSync(dir).forEach((entry) => { | ||
const entryPath = path.resolve(dir, entry); | ||
if (fs.lstatSync(entryPath).isDirectory()) { | ||
entries[entry] = walk(entryPath); |
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.
won't files with same names in different dirs clash here? should this be dir + entry
? (I'm not actually sure, hence asking)
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.
ummm...i don't think so? Because they'll be like
- parent
| - child
| - child2
| - child
should become
{
child: <Buffer>
child2: {
child: <Buffer>
}
}
right?
packages/wranglerjs-compat-webpack-plugin/src/__tests__/index.spec.ts
Outdated
Show resolved
Hide resolved
This removes the timeout we have for custom builds. We shouldn't be applying this timeout anyway, since it doesn't block wrangler, just the user themselves. Further, in #759, we changed the custom build's process stdout/stderr config to "pipe" to pass tests, however that meant we wouldn't see logs in the terminal anymore. This patch removes the timeout, and brings back proper logging for custom builds.
…838) This removes the timeout we have for custom builds. We shouldn't be applying this timeout anyway, since it doesn't block wrangler, just the user themselves. Further, in #759, we changed the custom build's process stdout/stderr config to "pipe" to pass tests, however that meant we wouldn't see logs in the terminal anymore. This patch removes the timeout, and brings back proper logging for custom builds.
For users that
type = webpack
this webpack plugin attempts to emulate wrangler 1's behavior