Bug 166987 - Add the support for nomodule attribute on script element
Summary: Add the support for nomodule attribute on script element
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML DOM (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-01-12 14:11 PST by Ryosuke Niwa
Modified: 2017-03-23 02:18 PDT (History)
8 users (show)

See Also:


Attachments
Patch (21.29 KB, patch)
2017-01-13 06:15 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (20.39 KB, patch)
2017-01-13 07:08 PST, Yusuke Suzuki
sam: review+
Details | Formatted Diff | Diff
Patch for landing (24.44 KB, patch)
2017-01-15 00:54 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2017-01-12 14:11:57 PST
See https://github.com/whatwg/html/pull/2261

We should add the support for nomodule attribute which prevents scripts from running now that we have the support for type=module.
Comment 1 Radar WebKit Bug Importer 2017-01-12 14:13:25 PST
<rdar://problem/30000539>
Comment 2 Yusuke Suzuki 2017-01-13 06:15:35 PST
Created attachment 298755 [details]
Patch
Comment 3 Sam Weinig 2017-01-13 06:53:37 PST
Comment on attachment 298755 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=298755&action=review

> Source/WebCore/html/HTMLScriptElement.idl:32
> +    attribute boolean noModule;

Can this be a [Reflect] attribute, and avoid the need for the explicit implementation?
Comment 4 Yusuke Suzuki 2017-01-13 07:01:36 PST
Comment on attachment 298755 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=298755&action=review

>> Source/WebCore/html/HTMLScriptElement.idl:32
>> +    attribute boolean noModule;
> 
> Can this be a [Reflect] attribute, and avoid the need for the explicit implementation?

Ah, I thought we need the implementation because of the name `noModule` JS property v.s. `nomodule` attribute name.
But it seems that we can use `Reflect` and `ImplementedAs`. Fixed.
Comment 5 Yusuke Suzuki 2017-01-13 07:05:26 PST
Comment on attachment 298755 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=298755&action=review

>>> Source/WebCore/html/HTMLScriptElement.idl:32
>>> +    attribute boolean noModule;
>> 
>> Can this be a [Reflect] attribute, and avoid the need for the explicit implementation?
> 
> Ah, I thought we need the implementation because of the name `noModule` JS property v.s. `nomodule` attribute name.
> But it seems that we can use `Reflect` and `ImplementedAs`. Fixed.

Oops, no, we can just use [Reflect] according to the CodeGenerator.pm.
Comment 6 Yusuke Suzuki 2017-01-13 07:08:16 PST
Created attachment 298757 [details]
Patch
Comment 7 Ryosuke Niwa 2017-01-13 13:55:36 PST
Comment on attachment 298757 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=298757&action=review

> Source/WebCore/dom/ScriptElement.h:112
> +    virtual bool noModuleAttributeValue() const = 0;

I think it’s better to call this hasNoModuleAttribute instead of noModuleAttributeValue.
Comment 8 Yusuke Suzuki 2017-01-14 09:17:12 PST
Comment on attachment 298757 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=298757&action=review

>> Source/WebCore/dom/ScriptElement.h:112
>> +    virtual bool noModuleAttributeValue() const = 0;
> 
> I think it’s better to call this hasNoModuleAttribute instead of noModuleAttributeValue.

If we change this to `hasNoModuleAttribute`, I think we should change deferAttributeValue, asyncAttributeValue.
Changed.
Comment 9 Yusuke Suzuki 2017-01-15 00:54:44 PST
Created attachment 298893 [details]
Patch for landing
Comment 10 Ryosuke Niwa 2017-01-23 21:46:07 PST
Did you ever land this patch? I can't find it on trac.
Comment 11 Yusuke Suzuki 2017-01-23 22:50:52 PST
(In reply to comment #10)
> Did you ever land this patch? I can't find it on trac.

yeah, I need to land it.
Comment 12 Yusuke Suzuki 2017-01-23 22:54:10 PST
Committed r211078: <http://trac.webkit.org/changeset/211078>
Comment 13 Serg Hospodarets 2017-03-23 02:18:17 PDT
Just want to make sure, will the first stable version of Safari with ES modules support also contain the "nomodule" addition?
Otherwise, the whole point may be missed: https://blog.hospodarets.com/native-ecmascript-modules-nomodule#comment-3218775686