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(terser): __filename not defined #1367

Merged
merged 2 commits into from Dec 21, 2022
Merged

Conversation

tada5hi
Copy link
Member

@tada5hi tada5hi commented Dec 6, 2022

Rollup Plugin Name: terser

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:
fixes #1366

Description

@tada5hi
Copy link
Member Author

tada5hi commented Dec 6, 2022

@shellscape would be great if you could have a look on this pr.
The addition to the codeowner file, doesn't seem to have worked.
It shows the following error: "Unknown owner on line 6: make sure @tada5hi exists and has write access to the repository"

carterworks added a commit to shammowla/alloy-gararge-week that referenced this pull request Dec 7, 2022
Disabled until rollup/plugins#1367 is merged.
carterworks added a commit to shammowla/alloy-gararge-week that referenced this pull request Dec 7, 2022
* Add alloy as a submodule

* Show the server response in the UI

* Optimize shared type files

* Build, with syntax highlighting

* Minify the output

Disabled until rollup/plugins#1367 is merged.

* Configure rollup a little
@UmamiAppearance
Copy link

UmamiAppearance commented Dec 9, 2022

Without having tested this, the worker thread seem to also take urls.

A simple switch between __filename and import.meta.url should be enough for the different entry points.


EDIT:
I was almost right. It works by passing import.meta.url to the URL constructor:

new Worker(new URL(import.meta.url));

But still, an extra import of fileURLToPath is not required.

@jonkoops
Copy link
Contributor

Got an alternative fix for this in #1374. Looks like import.meta.url is transpiled when compiling to CommonJS, so it should remain backwards compatible.

@tada5hi
Copy link
Member Author

tada5hi commented Dec 16, 2022

Got an alternative fix for this in #1374. Looks like import.meta.url is transpiled when compiling to CommonJS, so it should remain backwards compatible.

can confirm 👍

CODEOWNERS Show resolved Hide resolved
packages/terser/src/module.ts Show resolved Hide resolved
@jonkoops
Copy link
Contributor

@tada5hi just FYI you can use a keyword in the description of your PR to link it to the issue (see the GitHub documentation). That way it will be closed automatically when the PR is merged.

@tada5hi
Copy link
Member Author

tada5hi commented Dec 21, 2022

@tada5hi just FYI you can use a keyword in the description of your PR to link it to the issue (see the GitHub documentation). That way it will be closed automatically when the PR is merged.

Oh cool, thank you 😊.

@shellscape
Copy link
Collaborator

OK are we in agreement this can be merged?

@jonkoops
Copy link
Contributor

This looks good, no objections from me 👍

@tada5hi
Copy link
Member Author

tada5hi commented Dec 21, 2022

@shellscape yes from my side.

@shellscape shellscape merged commit 7259979 into rollup:master Dec 21, 2022
@shellscape
Copy link
Collaborator

Thanks all. This will be automatically published shortly.

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.

[@rollup/plugin-terser] esm module uses __filename
8 participants