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

Distinguish between module and classic scripts #5398

Closed
wants to merge 1 commit into from
Closed

Conversation

devongovett
Copy link
Member

@devongovett devongovett commented Nov 29, 2020

Whew, this turned out way bigger than I expected... 😂

This changes the way script tags are treated in HTML to distinguish between classic scripts and ES modules, improves Parcel's web compatibility, and introduces automatic modern-by-default optimizations for production builds.

  • import, export, require, and all Node polyfill features are now only allowed in <script type="module"> contexts (both inline and linked). Classic <script> elements can still have dynamic import(), Worker, SharedWorker, and navigator.serviceWorker.register dependencies. This matches what browsers support more closely, and allows better interoperability with legacy scripts where e.g. require being processed by Parcel was unexpected.
  • Classic <script> now works just like the browser - global variables are truly global. The bundle is not wrapped in a function, so top-level variables are accessible between scripts on the page, in HTML event handler attributes, etc. See e.g. Possible Bug: Parcel is commenting out my JS #5378. By default, when there are no dependencies, the script is not processed at all by Parcel. However, when an async dependency is seen that may require us to inject a runtime, we do wrap the bundle in a function to avoid unintended globals, but the globals seen in the original source are hoisted for compatibility.
  • In addition, <script type="module"> is now automatically compiled to both a modern ES module version and also a legacy <script nomodule> version if the browser targets do not all support ES modules natively. If no browserslist or target config is specified, then only a modern ES module version is output. For inline scripts, only a single script is output in order to avoid duplicating content - a modern version if all targets support it, otherwise legacy.
  • The same rules also apply in other scenarios, including web workers and service workers. These now require the type: 'module' option just like in the browser to use ES modules, otherwise they will behave just like as described above for classic scripts. Workers are compiled to ES modules natively if all browser targets support them, otherwise they are compiled to a classic script instead.

When an import or export is found in a classic script, we show an error message that indicates both where the dependency is, as well as where the environment was originally instantiated.

image

This also resulted in adding support for multiple contexts at once in an environment. For example, an asset may have both "browser" and "script" as its environment. This may have other uses later on. I also added a method to the environment object to allow easier checking if it supports a particular feature. There's basic data for things like esmodules, dynamic imports, etc. so far, but ideally we'd get this data from somewhere else that we don't need to maintain ourselves. Finally, environments also now support a loc property to track the source location where they were originally created, mostly for error messaging purposes.

One other change due to the multi-environment support is that I changed the config format to no longer configure runtimes per context, and instead just have a single pipeline. The reality was that all of the contexts used the same runtime plugin anyway, and if the runtimes are context specific, they can check internally themselves.

Close T-554, Close T-136

@height
Copy link

height bot commented Nov 29, 2020

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

@@ -0,0 +1,198 @@
const {Parser} = require('htmlparser2');

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will be sending a PR to posthtml for this

@@ -36,6 +36,8 @@ export async function parse({
strictMode: false,
sourceType: 'module',
plugins: ['exportDefaultFrom', 'exportNamespaceFrom', 'dynamicImport'],
// $FlowFixMe
startLine: asset.meta.loc?.start.line,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This offsets location information for both error messages and source maps of inline JS assets inside HTML so it starts at the correct line number of the original file

seen.add(copy);
}

attrs.src = asset.addURLDependency(attrs.src, {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A thought: both of these dependencies count against the parallel request limit, but only one will actually be downloaded. How should we represent that in the graph? 🤔

@mischnic
Copy link
Member

mischnic commented Dec 3, 2020

I think this should resolve #333?

Comment on lines +54 to +67
for (let context of ctx) {
switch (context) {
case 'node':
case 'electron-main':
case 'electron-renderer':
includeNodeModules = false;
break;
case 'browser':
case 'web-worker':
case 'service-worker':
default:
includeNodeModules = true;
break;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it depends on the order of the contexts? Maybe actually throw an error if there are conflicting values?

);
}

let prepareCode = (code, b) => {
Copy link
Member

@mischnic mischnic Jan 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we do #5548 instead?

prepareCode shouldn't be necessary anymore

@@ -615,7 +620,7 @@ export default class BundleGraph {
visitedBundles.add(descendant);
if (
descendant.type !== bundle.type ||
descendant.env.context !== bundle.env.context
!isSubset(descendant.env.context, bundle.env.context)
Copy link
Member

@mischnic mischnic Jan 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isSubset() checks a subset of b OR b subset of a. But doesn't the previous "context changes" translate to "child context contains something that the parent doesn't support"? So only child not subset of parent?

(so context gets more specific/refined vs is widened)

@mischnic
Copy link
Member

mischnic commented Jan 1, 2021

Are JS entry points assigned the module context?

@devongovett
Copy link
Member Author

Are JS entry points assigned the module context?

At the moment they have neither script or module. Actually, module is pretty much unused, we only check for script.

@mischnic
Copy link
Member

In the longer term, this could enable us to do something like Webpack 5's strict module mode (e.g. because of type: "modlue" in package.json) which disables some deprecated features.

@devongovett
Copy link
Member Author

Closing in favor of #6247

@devongovett devongovett closed this May 9, 2021
@mischnic mischnic deleted the html-script branch May 13, 2021 16:57
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.

2 participants