Closed
Bug 1330688
Opened 7 years ago
Closed 6 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)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: mossop, Assigned: jonco)
References
Details
Attachments
(1 file, 2 obsolete files)
4.40 KB,
patch
|
flod
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•7 years ago
|
Blocks: harmony:modules
Assignee | ||
Comment 1•7 years ago
|
||
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
Reporter | ||
Comment 2•7 years ago
|
||
Yeah, some message that the module could not be found would have been really useful for tracking down the problem.
Updated•7 years ago
|
Priority: -- → P3
Assignee | ||
Updated•7 years ago
|
Comment 3•7 years ago
|
||
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?
Assignee | ||
Comment 4•6 years ago
|
||
Testing with: data:text/html,%3Cscript type=module src=http://www.example.com/missing%3E%3C%2Fscript%3E
Assignee | ||
Comment 5•6 years ago
|
||
Patch to change console messages to refer to either a script or a module.
Assignee: nobody → jcoppeard
Attachment #8935811 -
Flags: review?(amarchesini)
Comment 6•6 years ago
|
||
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 7•6 years ago
|
||
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 8•6 years ago
|
||
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-
Assignee | ||
Comment 9•6 years ago
|
||
(In reply to Francesco Lodolo [:flod] from comment #8) Thanks for that, I'll redo the patch so that this can be localised correctly.
Assignee | ||
Comment 10•6 years ago
|
||
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)
Updated•6 years ago
|
Attachment #8936488 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 11•6 years ago
|
||
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 12•6 years ago
|
||
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?
Assignee | ||
Comment 13•6 years ago
|
||
(In reply to Francesco Lodolo [:flod] from comment #12) No reason, I'll fix that.
Assignee | ||
Comment 14•6 years ago
|
||
Attachment #8936488 -
Attachment is obsolete: true
Attachment #8936488 -
Flags: review?(francesco.lodolo)
Attachment #8937425 -
Flags: review?(francesco.lodolo)
Comment 15•6 years ago
|
||
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+
Comment 16•6 years ago
|
||
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
Comment 17•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8e6356ea82b2
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in
before you can comment on or make changes to this bug.
Description
•