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

fix: broken webpack mangling of harmony imports in production mode #223

Merged
merged 1 commit into from
Jul 7, 2023

Conversation

vilnytskyi
Copy link
Contributor

There is an issue when bundling code with imported aurelia dependency via harmony import. The code like:

import { ConsoleAppender } from 'aurelia-logging-console'
console.log(ConsoleAppender)
bundles into something like:

var a=r("aurelia-logging-console")
console.log(a.A)
while aurelia-logging-console isn't mangled itself to match the export names, so i.I is undefined.

This fix seem to work for all such cases but I am not sure if the original approach was written in such way for some specific cases which I might not be aware of so better to be investigated by core team.There is an issue when bundling code with imported aurelia dependency via harmony import. The code like:

import { ConsoleAppender } from 'aurelia-logging-console'
console.log(ConsoleAppender)
bundles into something like:

var a=r("aurelia-logging-console")
console.log(a.A)
while aurelia-logging-console isn't mangled itself to match the export names, so i.I is undefined.

This fix seem to work for all such cases but I am not sure if the original approach was written in such way for some specific cases which I might not be aware of so better to be investigated by core team.

Credits to @alexander-akait for pointing this out!

@bigopon
Copy link
Member

bigopon commented Jun 10, 2023

On any module that is referenced via PLATFORM.moduleName, which under the hood AureliaDependency, there' 2 things that need to happen:

  • the module must not be concatenated, and the module name must be preserved
  • all the exports must be preserved as is (no mangled)

The existing code is to make sure the 2nd part work. So will changing it to EXPORTS_OBJECT_REFERENCED make it work the same way?

@alexander-akait is there a better way to express the above requirement to webpack? At the moment, with AureliaDependency, which extends ModuleDependency, it results in non harmony import (legacy/requirejs), which causes problems.

An example of the issues is like in my analysis here #206 (comment)

image

In the above code block, line 5 results in a reference to @casl/ability/dist/umd/index.js instead of the module one at @casl/ability/dist/es6m/index.js

@alexander-akait
Copy link

@bigopon

On any module that is referenced via PLATFORM.moduleName, which under the hood AureliaDependency, there' 2 things that need to happen:

the module must not be concatenated, and the module name must be preserved
all the exports must be preserved as is (no mangled)

In such case this PR fix it.

Also in future we can implement more optimizations for commonjs, we don't implement all of them, because many of them are very rare.

But if you want to keep all exports and don't mangle it you should return nothing, so EXPORTS_OBJECT_REFERENCED is [[]], so you say webpack (siplified) - yes, we have export, but we don't want to change an export

@bigopon bigopon merged commit 492e434 into aurelia:master Jul 7, 2023
@bigopon
Copy link
Member

bigopon commented Jul 7, 2023

This has been released as 5.0.6
Thanks @vilnytskyi @alexander-akait

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.

3 participants