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

fix some problems related to module #22

Merged
merged 12 commits into from
Aug 2, 2022
Merged

fix some problems related to module #22

merged 12 commits into from
Aug 2, 2022

Conversation

h-takeyeah
Copy link

@h-takeyeah h-takeyeah commented Jul 6, 2022

変更点

コピーしてきた2つのファイルを含むThree.jsのバージョンは現時点の最新リリース[r142](https://github.com/mrdoob/three.js/tree/r142)です。もともとのjs/*Loader.jsやbuild/three.module.jsのバージョンが分からなかったので最新リリースを持ってきました。なのでバージョンの不整合が起きています。今の所エラーは無いですが、これを期にすべてアップデートするのも吉だと思います。

importmapについて

importmapを使用しているのはThree.jsのソースからコピーしてきている「Three.jsのコアには含まれない機能のためのJavaScriptファイル」を無修正で使いたいからです。ここでいう「Three.jsのコア」はbuild/three.module.jsのことであり、「含まれない機能」はLoaderやControlers等Three.jsのexamplesディレクトリに含まれるようなファイルのことです。ドキュメントにも以下のように表現されています。

Not all features are accessed through the three entrypoint. Other popular parts of the library — such as controls, loaders, and post-processing effects — must be imported from the examples/jsm subfolder.


こうしたファイルを今まではコピーしてきた後でimport {} from 'three'の部分をimport {} from '../build/three.module.js'へ手作業で書き換えていたのだと思います。どうやっていたのか私は知らないのでここは想像になります。こうしないとブラウザがthree.module.jsを解決できないことは周知の事実だと思います。

Uncaught TypeError: モジュール指定 “three” の解決時にエラーが発生しました。モジュール指定の相対パスは “./” または “../”, “/” のいずれかで始まらなければなりません。

書き換える作業はなんのことはないといえばなんのことはない作業ですが、書き換えたことによるバグを減らすには書き換えなければいいので、依存は増えますが、あえてimpormapを使うことを提案します(こういうことはIssueで書くべきですね、、、)。もちろんメリットだけではないので意見をいただければと思います。

動作確認に用いた環境

Arch Linux (x86_64), Firefox 102.0, Google Chrome 103.0.5060.53(Official Build)


close #21

arkwnet and others added 10 commits June 19, 2021 23:12
Ver.1.1.1 : Fix tooltip, Revision of texts
Ver.2.0.0
Fix three.module.js path
Ver.2.1.0
to fix `Uncaught TypeError: Failed to resolve module specifier "three". Relative references must start with either "/", "./", or "../".`
@h-takeyeah h-takeyeah requested a review from arkwnet July 6, 2022 08:50
importmap reference: https://github.com/WICG/import-maps
importmap does not work in Firefox
@h-takeyeah
Copy link
Author

  • 74a8016
    • Firefoxでimportmapがサポートされていないのでpolyfillを追加(いずれjsディレクトリに置くかも知れませんが一旦CDN)
    • Mozilla の立場 Mozilla Position
    • Firefoxでの既知のエラー
      • Firefoxではこのpolyfillが動作する前にブラウザネイティブのモジュール解決機構が動作して失敗し、そのことがコンソールに出力されますが、その後polyfillがきちんと解決してくれるので問題はありません。ちょっと気持ち悪いですけど。

このコミットをFirefoxで見たときのスクショ

firefox after image

1行目はよく分かりません(消せない)。2行目がブラウザネイティブのモジュール解決機構が失敗している様子。3行目は関係ないです。4行目までにpolyfillがモジュールを解決して2行目のエラーは無視してOKである旨を4行目に表示しています。

比較のためにpolyfill無しの1つ前のコミット(6fb8d0b)をFirefoxで見た様子とChromeで見た様子を載せておきます。なおChromeはネイティブでimportmapをサポートしているのでpolyfillしてもしなくても変わりません。

Firefox(読み込み中の画面で止まる)
firefox before image
Chrome(正常に動作)
chrome before image


警告を消す方法

利用しているpolyfillのes-module-shimsには2つのモードがあります。今はpolyfillモードですが、shimモードにするとネイティブのモジュール解決機構が走らなくなるので2行目のエラーは消えます。ただしshimモードにすると今度は(js/main.jsから監視できるタイミングでは)DOMContentLoadedイベントが発火しなくなるという問題があります。この場合はjs/main.jsでDOMContentLoadedイベントにリスナを登録する前にreadyStateをチェックするなど工夫が必要です。

もしくは現状でもjs/*Loader.jsで行われていますが ef35db4 でコピーしてきたjs/*Controls.jsに以下のようなカスタマイズをすればpolyfillなしでも動きます。あと1行目のasm.jsのエラーも消えます。

// (変更前)
import {
        EventDispatcher,
...
} from 'three';
// (変更後)
import {
        EventDispatcher,
...
} from '../build/three.module.js';

@h-takeyeah
Copy link
Author

h-takeyeah commented Jul 6, 2022

es-module-shimsをpolyfillモードで使用したとき、js/main.jsで登録したDOMContentLoadedイベントで呼ばれるはずのコールバック(app.init())が呼ばれていませんでした。vhc.html側でwindowとdocumentそれぞれにイベントリスナをつけて調べてみました。以下に実行結果を示します。

image

そもそもDOMContentLoadedイベントはdocumentで発生するイベントであり、以下のコードは親に向けて伝播してきた(バブリングしてきた)イベントを見つけてコールバックを呼びます。第三引数のuseCaptureを省略しているのでデフォルトのfalseが設定され、このdoSomethingというコールバックはバブリングフェーズで呼ばれることになります。

window.addEventListener('DOMContentLoaded', doSomething);

上のスクショを見れば分かるように、DOMContentLoadedイベントの発火にともなって呼ばれたコールバック(最初の2つ)はどちらもバブリングフェーズで呼ばれています(document→windowの順であることからも分かる)。しかしこの時点ではpolyfillによるモジュール解決が完了していない(緑文字のメッセージがまだ出ていない)のでjs/main.jsでコールバックを登録できていない状態です。その後モジュールが解決され、上に示したwindow.addEventListener('DOMContentLoaded', doSomething)がjs/main.jsで実行されます。これでコールバックが登録されました。当然ながら今登録されたこのコールバックも呼ばれるのはバブリングフェーズのときです。その後polyfillがブラウザの代わりにDOMContentLoadedイベントを改めて発火してくれます(最後のやつ)。

polyfillによる実際のコードはこの辺です。 https://github.com/guybedford/es-module-shims/blob/main/src/es-module-shims.js#L490,#L493

document.dispatchEvent(new Event('DOMContentLoaded'));

ただしEventコンストラクタにイベントの名前だけ渡して第二引数のオプションを何も渡さなかった場合、生成されたイベントはバブルしません(MDN)。つまり子要素から親要素に向かって伝播しません。したがって先ほどバブリングフェーズで呼ばれるように登録したコールバックはいつまで経っても呼ばれません。

これを解決するにはキャプチャリングフェーズ(親要素から子要素の方向に順にコールバックを呼んでいく段階)でコールバックを呼ぶようにリスナを登録するか、windowではなくdocumentに直接リスナをつけるかする必要があります。

またはこのような方法もあります。

function doSomething() {
  console.info('DOM loaded');
}

if (document.readyState === 'loading') {  // Loading hasn't finished yet
  document.addEventListener('DOMContentLoaded', doSomething);
} else {  // `DOMContentLoaded` has already fired
  doSomething();
}

@h-takeyeah
Copy link
Author

  • 2fd6f36
    • 一つ前の私のコメントで示した通り、windowにリスナがついているとpolyfillによるdocument.dispatchEvent(new Event('DOMContentLoaded'))に反応できないので、windowの代わりにdocumentにリスナをつけるように変更

このコミットより前のコミットがapproveされなかったとしてもこのコミットは残していいと思います。なぜならDOMContentLoadedイベントを発火させているのはブラウザか今回の場合はpolyfill用のes-modules-shimであり、どちらもdocument.DOMContentLoadedイベントでありdocumentにリスナをつけるのが妥当だからです。

@h-takeyeah h-takeyeah changed the title #21 fix problems related to module fix some problems related to module Jul 6, 2022
@arkwnet arkwnet self-assigned this Aug 1, 2022
@arkwnet arkwnet closed this Aug 1, 2022
@arkwnet arkwnet reopened this Aug 2, 2022
@arkwnet arkwnet changed the base branch from master to develop-v2 August 2, 2022 13:17
@arkwnet arkwnet merged commit 56a9cfb into develop-v2 Aug 2, 2022
@h-takeyeah h-takeyeah deleted the modularize branch October 28, 2022 05:25
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.

WARNING: Multiple instances of Three.js being imported
2 participants