Implement support for classic scripts and automatic module/nomodule #6247

devongovett · 2021-05-09T04:05:08Z

This implements #5398 on top of the new SWC-based JS transformer. It takes a slightly different approach: rather than adding support for multiple contexts to the environment, it adds a sourceType field, which can either be script for classic scripts, or module for ES modules.

The other difference from the original implementation is the way scripts hoist their top-level variables into the global scope. In the previous implementation, this was done by attempting to find all top-level vars and assign them to the global object. In this implementation, the code for the entry asset of script bundles itself is moved outside the bundle wrapper. Any require calls that the entry asset might do (e.g. for runtimes) are replaced with a parcelRequire call.

Otherwise, everything is the same as the previous PR. See there for details.

~~The posthtml parser fork for adding location information will be removed once posthtml/posthtml-parser#63 is merged and released.~~ DONE

Close T-136. Fixes #333. Closes #3011.

height · 2021-05-09T04:05:10Z

This pull request has been linked to and will mark 2 tasks as "Done" when merged:

~~T-136 Compile a single script into multiple with module/nomodule automatically~~ (unlink task)
~~T-554 Remove context mapping from runtimes config~~ (unlink task)

devongovett · 2021-05-09T04:10:52Z

packages/transformers/js/src/JSTransformer.js

    });

    let convertLoc = loc => ({
      filePath: relativePath,
      start: {
-        line: loc.start_line,
+        line: loc.start_line + Number(asset.meta.startLine ?? 1) - 1,


This is kind of a hack. Should we do something in core for this?

devongovett · 2021-05-09T04:14:52Z

packages/transformers/js/src/lib.rs

+                    decls: &decls,
+                    used_env: &mut result.used_env
+                  },
+                  config.source_type != SourceType::Script


Should process.env be supported in scripts?

I guess it would be nice? But we could also add that later if it comes up. Was there a reason you disabled it?
The only thing we can't support if course is the process Node global replacement.

Mainly I was just trying to make scripts as close to real browser JS as possible, and not enable any CommonJS/Node features.

# Conflicts: # packages/core/core/src/Environment.js # packages/core/core/test/Environment.test.js # packages/core/core/test/TargetRequest.test.js # packages/transformers/html/src/dependencies.js # packages/transformers/js/core/src/global_replacer.rs # packages/transformers/js/src/JSTransformer.js

packages/core/types/index.js

mischnic · 2021-06-17T22:00:31Z

packages/transformers/html/src/dependencies.js

+        delete attrs.type;
+        attrs.nomodule = '';
+        attrs.src = asset.addURLDependency(attrs.src, {
+          // Keep in the same bundle group as the HTML.
+          priority: 'parallel',
+          env: {
+            sourceType,
+            outputFormat: 'global',
+            loc,
+          },
+        });


Do these statements do anything? attrs is a shallow clone and it was already merged into copy

attrs is referenced by copy, so this is modifying the attrs of the copy.

packages/core/core/src/Environment.js

packages/core/core/src/public/Environment.js

packages/core/types/index.js

wbinnssmith · 2021-06-17T22:28:03Z

packages/core/core/test/Environment.test.js

@@ -6,7 +6,7 @@ import {createEnvironment} from '../src/Environment';
 describe('Environment', () => {
  it('assigns a default environment with nothing passed', () => {
    assert.deepEqual(createEnvironment(), {
-      id: '1051b306b4fe6e64',
+      id: '2163092a3dd83a5f',


Would be neat if we used/made something along the lines of Jest's stringMatching matcher: https://jestjs.io/docs/expect#expectstringmatchingstring--regexp

wbinnssmith · 2021-06-17T22:30:56Z

packages/core/integration-tests/test/sourcemaps.js

@@ -428,7 +428,7 @@ describe('sourcemaps', function() {
      source: inputs[0],
      generated: raw,
      str: 'const local',
-      generatedStr: 'const o',
+      generatedStr: 'const t',


mischnic · 2021-06-17T22:39:10Z

packages/transformers/js/core/src/global_replacer.rs

@@ -70,6 +70,7 @@ impl<'a> Fold for GlobalReplacer<'a> {
              attributes: None,
              is_optional: false,
              is_helper: false,
+              source_type: Some(SourceType::Module),


Should this be None as in fs.rs?

file dependencies don't need a source type, but in this case it's referring to a require that should be treated as a module.

wbinnssmith

LGTM

wbinnssmith · 2021-06-17T22:32:00Z

packages/packagers/js/src/CJSOutputFormat.js

@@ -148,7 +148,7 @@ export class CJSOutputFormat implements OutputFormat {
    return [res, lines];
  }

-  buildBundlePostlude(): string {
-    return '';
+  buildBundlePostlude(): [string, number] {


nit: Maybe a type alias or a comment explaining what these values in the return type mean?

rhendric · 2021-06-25T02:02:43Z

Found my way here from the new "Browser scripts cannot have imports or exports" error. Why does this restriction exist, and can it be suppressed if we know that Parcel is going to bundle away all of the import statements?

devongovett · 2021-06-25T04:51:09Z

You just need to add type="module" to your script tag and then it should work fine. If your browser targets defined in your browserslist don't all support modules natively, Parcel will auto generate a nomodule version as well.

rhendric · 2021-06-25T04:59:34Z

What if I only want to produce one bundle for all browsers? If everything's being bundled together, what does it matter if the bundle is being loaded as a module or not? Seems unusually opinionated for Parcel to force there to be two essentially identical versions of the bundle; what are we getting for this?

devongovett · 2021-06-25T05:31:33Z

The main reason is to ship less code to most browsers. 92.64% of global web traffic is via a browser that supports ES modules natively. This also means that we can ship a lot more modern JS features like classes etc. without transpilation which results in reduced bundle sizes. Basically, unless you have to support IE 11 still, you will get one bundle anyway, and if you do, then most of your users will need to download less code, and only IE 11 users will need to download the larger legacy bundle.

rhendric · 2021-06-25T18:50:14Z

Hmm, okay. I still don't see why Parcel couldn't just support script tags without the module type by doing exactly what it does to generate a nomodule bundle, or what transpilation of other JS features has to do with modules and imports/exports (which are completely eliminated in bundles regardless of what features the browser supports), but I can try targeting modules and seeing if that causes any problems for me.

devongovett · 2021-06-25T19:14:08Z

Yeah it's because scripts behave differently in other ways as well. Whereas modules are self contained, scripts run in the global context meaning any variables created become global. This might be seen as a bad idea but there are lots of legacy libraries like jquery that rely on this assumption. The goal of Parcel is to work exactly like if you ran your code natively in a browser but also make your code portable to older browsers as well if needed. Treating scripts as modules in Parcel v1 was a mistake that we're trying to correct in v2.

rhendric · 2021-06-25T19:36:58Z

Ah, so this is a way to support

<script src="jquery.js"></script>
<script type="module" src="app.js"></script>

such that on old browsers, jquery.js is loaded as-is but app.js is wrapped in an IIFE or something to keep it from adding to the global scope?

devongovett · 2021-06-25T20:15:54Z

Yeah exactly. It solves two separate problems that we've had for a long time - legacy script support, and automatic modern/legacy builds for reduced bundle sizes. It'll be a transition for everyone to get used to for sure. If you think there's an improvement we can make to the error message, we'd love to hear suggestions. Maybe we can add a link to the docs with some more info or something.

ivoba · 2021-06-30T11:49:28Z

This is awesome!
@devongovett is this already in 2.0.0-nightly.746 ?

I have classic imports but they still seem to be wrapped as modules.

    <script src="/node_modules/jquery/dist/jquery.min.js"></script>
    <script src="/js/jquery.cycle.lite.js"></script>

which results in:

$(...).cycle is not a function

mischnic · 2021-06-30T12:51:51Z

is this already in 2.0.0-nightly.746 ?

Yes

$(...).cycle is not a function

Can you open an issue for that with a full code sample (not sure if there's more needed that you already posted)?

devongovett commented May 9, 2021

View reviewed changes

devongovett mentioned this pull request May 9, 2021

Distinguish between module and classic scripts #5398

Closed

Base automatically changed from swc-scope-hoisting to v2 May 10, 2021 00:13

devongovett added 6 commits May 31, 2021 21:50

Implement support for classic scripts and automatic module/nomodule

320564b

Fix unit tests

fbecf26

Make flow happy

96690cd

fixup

53e4107

Remove posthtml-parser fork

5e690f3

formatting

ddf378a

devongovett force-pushed the script-module branch from 1221253 to ddf378a Compare June 1, 2021 05:20

devongovett added 3 commits May 31, 2021 22:22

flow

5caa992

fixup

c99ef2c

mischnic reviewed Jun 17, 2021

View reviewed changes

packages/core/types/index.js Show resolved Hide resolved

mischnic reviewed Jun 17, 2021

View reviewed changes

wbinnssmith reviewed Jun 17, 2021

View reviewed changes

packages/core/core/src/Environment.js Outdated Show resolved Hide resolved

packages/core/core/src/Environment.js Outdated Show resolved Hide resolved

packages/core/core/src/public/Environment.js Outdated Show resolved Hide resolved

packages/core/types/index.js Show resolved Hide resolved

wbinnssmith reviewed Jun 17, 2021

View reviewed changes

mischnic reviewed Jun 17, 2021

View reviewed changes

devongovett added 2 commits June 18, 2021 12:37

Fix cross origin ESM workers

516c5bd

Flow improvements

e776694

wbinnssmith approved these changes Jun 18, 2021

View reviewed changes

Will Binns-Smith and others added 4 commits June 18, 2021 13:45

Merge branch 'v2' into script-module

0ec6ad7

Fix merge

174752d

format

3c9df3c

normalize path on windows

b1bffcf

devongovett merged commit dfb179b into v2 Jun 19, 2021

devongovett deleted the script-module branch June 19, 2021 00:52

This was referenced Jul 8, 2021

Efficient IE11 support #1168

Closed

[Parcel 2] build breaks inline script tag for external library #6213

Closed

Incomplete refactoring of function names #2107

Closed

thewilkybarkid mentioned this pull request Aug 20, 2021

Parcel 2: @parcel/transformer-html: Source indices must be monotonic #6672

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement support for classic scripts and automatic module/nomodule #6247

Implement support for classic scripts and automatic module/nomodule #6247

devongovett commented May 9, 2021 •

edited

Loading

height bot commented May 9, 2021 •

edited

Loading

devongovett May 9, 2021

devongovett May 9, 2021

mischnic Jun 17, 2021 •

edited

Loading

devongovett Jun 18, 2021

mischnic Jun 17, 2021

devongovett Jun 18, 2021

wbinnssmith Jun 17, 2021

wbinnssmith Jun 17, 2021

wbinnssmith Jun 17, 2021

mischnic Jun 17, 2021

devongovett Jun 18, 2021

wbinnssmith left a comment

wbinnssmith Jun 17, 2021

rhendric commented Jun 25, 2021

devongovett commented Jun 25, 2021 •

edited

Loading

rhendric commented Jun 25, 2021

devongovett commented Jun 25, 2021

rhendric commented Jun 25, 2021

devongovett commented Jun 25, 2021

rhendric commented Jun 25, 2021

devongovett commented Jun 25, 2021

ivoba commented Jun 30, 2021

mischnic commented Jun 30, 2021

Implement support for classic scripts and automatic module/nomodule #6247

Implement support for classic scripts and automatic module/nomodule #6247

Conversation

devongovett commented May 9, 2021 • edited Loading

height bot commented May 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mischnic Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbinnssmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhendric commented Jun 25, 2021

devongovett commented Jun 25, 2021 • edited Loading

rhendric commented Jun 25, 2021

devongovett commented Jun 25, 2021

rhendric commented Jun 25, 2021

devongovett commented Jun 25, 2021

rhendric commented Jun 25, 2021

devongovett commented Jun 25, 2021

ivoba commented Jun 30, 2021

mischnic commented Jun 30, 2021

devongovett commented May 9, 2021 •

edited

Loading

height bot commented May 9, 2021 •

edited

Loading

mischnic Jun 17, 2021 •

edited

Loading

devongovett commented Jun 25, 2021 •

edited

Loading