Closed Bug 1330688 Opened 3 years ago Closed 2 years ago

Using the wrong name when importing an ES6 module breaks the script and doesn't log any error

Categories

(Core :: DOM: Core & HTML, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: mossop, Assigned: jonco)

References

Details

Attachments

(1 file, 2 obsolete files)

I had a module that looked like this:

    alert('foo');
    import foo from './store';
    alert('foo');

I had left off the ".js" extension from the module to import. I saw neither of the alerts appear and no error in the webconsole to help me track down the error.
Module imports are loaded before the script is executed so you won't see either alert, but we could produce an error to make problems easier to track down.
Component: JavaScript Engine → DOM: Core & HTML
Yeah, some message that the module could not be found would have been really useful for tracking down the problem.
Priority: -- → P3
Blocks: modules-0
No longer blocks: harmony:modules
Since bug 247996 the console shows:
> Loading failed for the <script> with source “http://0.0.0.0:8000/store”. test.html:1

We should probably change <script> to module?
Testing with: data:text/html,%3Cscript type=module src=http://www.example.com/missing%3E%3C%2Fscript%3E
Attached patch bug1330688-module-console-error (obsolete) — Splinter Review
Patch to change console messages to refer to either a script or a module.
Assignee: nobody → jcoppeard
Attachment #8935811 - Flags: review?(amarchesini)
Comment on attachment 8935811 [details] [diff] [review]
bug1330688-module-console-error

Review of attachment 8935811 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/script/ScriptLoadRequest.h
@@ +254,5 @@
>                              const char* aName,
>                              uint32_t aFlags);
>  
> +const char16_t*
> +ScriptKindName(ScriptKind kind);

can you make this a static method in ScriptLoadRequest?
Attachment #8935811 - Flags: review?(amarchesini) → review+
Comment on attachment 8935811 [details] [diff] [review]
bug1330688-module-console-error

flod, can you check the dom.properties part of this patch?
Attachment #8935811 - Flags: review?(francesco.lodolo)
Comment on attachment 8935811 [details] [diff] [review]
bug1330688-module-console-error

Review of attachment 8935811 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, all the string IDs need to change, since you changed the text.
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings

Also:
* Use %1$S, %2$S, etc. when there's more than one placeholder
* If there's a placeholder, comment should explain what they are (what's going to replace them). You changed all the strings, and now they have comments that are not relevant anymore (there's no <script> in the text)
Attachment #8935811 - Flags: review?(francesco.lodolo) → review-
(In reply to Francesco Lodolo [:flod] from comment #8)
Thanks for that, I'll redo the patch so that this can be localised correctly.
I updated the patch to make separate messages for module errors so these can be localised correctly.
Attachment #8935811 - Attachment is obsolete: true
Attachment #8936488 - Flags: review?(amarchesini)
Attachment #8936488 - Flags: review?(amarchesini) → review+
Comment on attachment 8936488 [details] [diff] [review]
bug1330688-module-console-error v2

Just wanted to check that this is OK now.
Attachment #8936488 - Flags: review?(francesco.lodolo)
Comment on attachment 8936488 [details] [diff] [review]
bug1330688-module-console-error v2

Review of attachment 8936488 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/locales/en-US/chrome/dom/dom.properties
@@ +343,3 @@
>  # LOCALIZATION NOTE: Do not translate "<script>".
>  ScriptSourceMalformed=<script> source URI is malformed: “%S”.
> +ModuleSourceMalformed=module source URI is malformed: “%S”.

Is there a reason for this string and ModuleSourceNotAllowed to start lowercase?
(In reply to Francesco Lodolo [:flod] from comment #12)
No reason, I'll fix that.
Attachment #8936488 - Attachment is obsolete: true
Attachment #8936488 - Flags: review?(francesco.lodolo)
Attachment #8937425 - Flags: review?(francesco.lodolo)
Comment on attachment 8937425 [details] [diff] [review]
bug1330688-module-console-error v3

Review of attachment 8937425 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, it looks good. 

Nice how hg diff still screws up in 2017 with UTF-8 characters appearing in the @@ line…
Attachment #8937425 - Flags: review?(francesco.lodolo) → review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e6356ea82b2
Report a better error in the console when a module load fails r=baku r=flod
https://hg.mozilla.org/mozilla-central/rev/8e6356ea82b2
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.