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.

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

chore: Misc cleaning #1659

Merged
merged 4 commits into from Feb 9, 2022
Merged

chore: Misc cleaning #1659

merged 4 commits into from Feb 9, 2022

Conversation

rschristian
Copy link
Member

@rschristian rschristian commented Feb 7, 2022

Sorry, yet another bit of backporting changes made in the v4/webpack 5 branches.

  • Added TS template tests, closes Add a test to build TS project #1315
  • "Fixed" push-manifest creation in non-esm environments (though this shouldn't have ever caused an actual issue, see below for example)
  • Removed some extraneous things that weren't in use, and tried to correct some random inconsistencies

Shouldn't be any functional changes or fixes.

@rschristian rschristian requested a review from a team as a code owner February 7, 2022 07:57
@changeset-bot
Copy link

changeset-bot bot commented Feb 7, 2022

🦋 Changeset detected

Latest commit: 00a3cd3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
preact-cli Patch

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

Comment on lines -25 to -49
it.skip('should hydrate routes progressively.', async () => {
let dir = await subject('progressive-hydration');
await build(dir);
const server = getServer(join(dir, 'build'), PORT);

// let page = await loadPage(chrome, `http://127.0.0.1:${PORT}/`);
const page = await chrome.newPage();

page.on('console', consoleMessage => {
// eslint-disable-next-line
console[consoleMessage.type()](consoleMessage.text());
});

await page.goto(`http://127.0.0.1:${PORT}/`);

// await waitUntilExpression(page, `window.booted`);
await sleep(500);

const mutations = await page.evaluate('window.mutations');

expect(mutations).toHaveLength(0);

server.server.close();
});

Copy link
Member Author

Choose a reason for hiding this comment

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

Has been skipped since it was added: #957

@@ -1,13 +1,16 @@
const { join } = require('path');
const { mkdirSync, symlinkSync } = require('fs');
const { mkdir, symlink } = require('fs').promises;
Copy link
Member Author

Choose a reason for hiding this comment

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

Should be ever so slightly faster with promises, and everything is already async anyways.

@@ -156,14 +158,12 @@ async function clientConfig(env) {
};
}

function getBabelEsmPlugin(config) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Besides a few spots in client-config, everywhere else we use env, rather than config. This has been slightly awkward to follow as I've done v5 stuff.

Comment on lines -296 to -299
new webpack.DefinePlugin({
'process.env.ADD_SW': config.sw,
'process.env.PRERENDER': config.prerender,
}),
Copy link
Member Author

Choose a reason for hiding this comment

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

Values are duplicated with prod config, so they can be moved up to common.

@rschristian rschristian marked this pull request as ready for review February 8, 2022 21:59
@@ -8,7 +8,7 @@ module.exports = (assets, isESMBuild = false, namedChunkGroups) => {
styles = [];
for (let filename in assets) {
if (!/\.map$/.test(filename)) {
if (/route-/.test(filename)) {
if (/^route-.*\.js$/.test(filename)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This created some junk output in push-manifest.json when ESM is disabled, like in our test suite. Wouldn't have ever been a problem, as these junk "routes" would never match to a URL when building the HTML, but certainly looked odd if any user took a peek.

{
  "/": {
    "bundle.2da73.css": {
      "type": "style",
      "weight": 1
    },
    "bundle.44866.js": {
      "type": "script",
      "weight": 1
    },
    "route-home.chunk.3cec8.js": {
      "type": "script",
      "weight": 0.9
    },
    "route-home.chunk.bcb8a.css": {
      "type": "style",
      "weight": 0.9
    }
  },
  "/.chunk.bcb8a.css": {
    "bundle.2da73.css": {
      "type": "style",
      "weight": 1
    },
    "bundle.44866.js": {
      "type": "script",
      "weight": 1
    },
    "route-home.chunk.bcb8a.css": {
      "type": "script",
      "weight": 0.9
    }
  },
  "/profile.chunk.6dd80.css": {
    "bundle.2da73.css": {
      "type": "style",
      "weight": 1
    },
    "bundle.44866.js": {
      "type": "script",
      "weight": 1
    },
    "route-profile.chunk.6dd80.css": {
      "type": "script",
      "weight": 0.9
    }
  },
  "/profile": {
    "bundle.2da73.css": {
      "type": "style",
      "weight": 1
    },
    "bundle.44866.js": {
      "type": "script",
      "weight": 1
    },
    "route-profile.chunk.ddf94.js": {
      "type": "script",
      "weight": 0.9
    },
    "route-profile.chunk.6dd80.css": {
      "type": "style",
      "weight": 0.9
    }
  }
}

@ForsakenHarmony ForsakenHarmony merged commit d452863 into master Feb 9, 2022
@ForsakenHarmony ForsakenHarmony deleted the chore/misc-cleaning branch February 9, 2022 16:27
@preact-bot preact-bot mentioned this pull request Feb 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a test to build TS project
2 participants