Skip to content

fix: provide link if auto-refresh disabled#62

Closed
midgleyc wants to merge 2 commits intoLoathing-Associates-Scripting-Society:mainfrom
midgleyc:main
Closed

fix: provide link if auto-refresh disabled#62
midgleyc wants to merge 2 commits intoLoathing-Associates-Scripting-Society:mainfrom
midgleyc:main

Conversation

@midgleyc
Copy link

@midgleyc midgleyc commented Jun 4, 2021

Using Firefox, you can set accessibility.blockautorefresh to true in about:config to block automatic refresh. This means that the newer Philter Manager shows a blank white screen.

This PR adds a link you can click if the refresh hasn't occurred.

I edited both the .ts and the .js files manually: editing the .ts file and running a build (npm run build) changed the assets files, but not the relay_Philter_Manager.js file.

@pastelmind
Copy link
Contributor

Apologies for the delay.

relay_Philter_Manager.js was not regenerated because of #60, which is fixed in #63. If you rebase your PR onto main and run yarn build again, all bundles should be updated correctly.

Also, we currently use Yarn v1 instead of NPM. For the sake of reproducibility, please use yarn run build instead of npm run build. (I am considering moving to NPM v7 instead of Yarn v2, but that is another matter.)

Would you like to do this yourself? If not, I can make another PR.

@midgleyc
Copy link
Author

Sure, rebased and rebuilt.

I think npm run build and yarn run build should do exactly the same thing: the script in package.json runs yarn, anyway.

@pastelmind
Copy link
Contributor

pastelmind commented Jun 10, 2021

It appears that yarn build results in different chunk names on my machine (Windows 10) and yours. The vendor chunk's hash is different, too. This doesn't make sense because no dependencies should have changed. What OS do you use?

I tried running yarn build on Ubuntu 20.04 (WSL) and it resulted in the same chunk names as those in your PR.

@midgleyc
Copy link
Author

Yes, exactly that: WSL on Windows 10. I'd expect it to be an issue of line endings.

Looking at release/relay/relay_Philter_Manager.js, that file has changed: lines from 10 through 80 (the copyright header through the __spreadArray function) have windows line endings, but everything else has linux line endings.

@pastelmind
Copy link
Contributor

Of course! @philter/common is built by tsc, which emits CRLF on Windows and LF on Linux. Since Rollup.js directly consumes it, the resulting chunk names are different.

If the contents of node_modules are also affected by platform-specific line endings, it also explains the hash change for the vendor chunk as well.

I'm not sure how to "fix" this...ugh. However, this is clearly beyond the scope of this PR. For now, I'll just accept it.

Thanks!

@midgleyc
Copy link
Author

Searching through the codebase for carriage returns, I find them only in release/relay/relay_Philter_Manager.js and LICENSE.

If I add "newLine": "CRLF" to packages/common/tsconfig.json, I can generate different hashes on build, and the generated file has more carriage returns. Not that many more, though! If I change it to "newLine": "LF" I don't get any fewer carriage returns.

I really don't know what's going on here. require('os').EOL is \n as expected, so I don't know where the thing generating carriage returns is getting them from.

@pastelmind
Copy link
Contributor

pastelmind commented Jun 11, 2021

It seems that object-assign is bundled using different identifiers on Windows and Linux.

This is from a diff of vendor.*.js.map generated in Windows and Linux:

-"names":["getOwnPropertySymbols","Object","hasOwnProperty","prototype","propIsEnumerable","propertyIsEnumerable","val","TypeError","C__Users_Phil_Documents_GitHub_philter_node_modules_objectAssign","assign","test1",
+"names":["getOwnPropertySymbols","Object","hasOwnProperty","prototype","propIsEnumerable","propertyIsEnumerable","val","TypeError","objectAssign","assign","test1",

My question is: How is the absolute path (C__Users_Phil_Documents_GitHub_philter_node_modules_objectAssign) sneaking into the build process?

Edit: It's not just object-assign...looks like Rollup loves using absolute paths on Windows. This is from an unminified vendor.*.js:

-var C__Users_Phil_Documents_GitHub_philter_node_modules_react = {exports: {}};
+var react = {exports: {}};

pastelmind added a commit that referenced this pull request Jun 11, 2021
Ensure that TypeScript emits files with LF line endings on all platforms
including Windows. This fix is needed only for @philter/common, whose
build artifacts are directly generated by TypeScript. Other packages use
Rollup, which always emits output with LF-endings.

Previously, TypeScript emitted bundles had CRLF line endings on Windows.
This was then consumed by @philter/manager and @philter/api. Since both
projects use Rollup, I didn't notice this in their output bundles.
However, it affected the sourcemaps, which had '\r'. Also, it caused
chunk hashes to be computed differently on Windows and Linux.

I discovered this while investigating #62.
pastelmind added a commit that referenced this pull request Jun 11, 2021
Ensure that TypeScript emits files with LF line endings on all platforms
including Windows. This fix is needed only for @philter/common, whose
build artifacts are directly generated by TypeScript. Other packages use
Rollup, which always emits output with LF-endings.

Previously, TypeScript emitted bundles had CRLF line endings on Windows.
This was then consumed by @philter/manager and @philter/api. Since both
projects use Rollup, I didn't notice this in their output bundles.
However, it affected the sourcemaps, which had '\r'. Also, it caused
chunk hashes to be computed differently on Windows and Linux.

I discovered this while investigating #62.
pastelmind added a commit that referenced this pull request Aug 23, 2021
Notes:

- Update Vite to 2.5.0, which fixes a bug that caused absolute paths of
  dependencies to leak into the bundle (and the sourcemap). This was
  discovered in pull request #62, investigated further in issue #75, and
  then remedied via monkeypatching Vite in PR #77.
  Now that Vite has fixed the bug, we can remove the monkeypatch.
  (`patch-package` is still needed to work with `react-virtualized`)
pastelmind added a commit that referenced this pull request Aug 23, 2021
Notes:

- Update Vite to 2.5.0, which fixes a bug that caused absolute paths of
  dependencies to leak into the bundle (and the sourcemap). This was
  discovered in pull request #62, investigated further in issue #75, and
  then remedied via monkeypatching Vite in PR #77.
  Now that Vite has fixed the bug, we can remove the monkeypatch.
  (`patch-package` is still needed to work with `react-virtualized`)
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.

2 participants