feat(@parcel/transformer-js): Rewrite `dirname`/`filename` to be relative to `asset.filePath` #7727

LekoArts · 2022-02-17T10:10:12Z

↪️ Pull Request

This adds a way of accomplishing #7611 - this functionality will be enabled by default in node environment.

💻 Examples

See linked issue, but TL;DR is:

Some code in e.g. <root>/index.ts has a ${__dirname}/data string in it.
When it'll be compiled to e.g. <root>/dist the path should be changed to:

const path = require("path")
const dirname = path.resolve(__dirname, "..")
return `${dirname}/data`

🚨 Test instructions

I've added a test to the integration tests, it has to have the node env (so e.g. engines: node key).

✔️ PR Todo

Added/updated unit tests for this change
Filled out test instructions (In case there aren't any unit tests)
Included links to related issues/PRs

height · 2022-02-17T10:10:13Z

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

packages/core/core/src/resolveOptions.js

packages/transformers/js/src/JSTransformer.js

mischnic

Looks good overall!

IMO relativeContext doesn't really convey that this is about __dirname and __filename 🤷‍♂️

There's a similar existing test here:

parcel/packages/core/integration-tests/test/javascript.js

Line 2646 in 47379bf

it('should insert global variables when needed', async function () {

So duplicate that and add a package.json with that new config option, then you can delete the example you've committed.

Regarding the path import: this is similar to how an import for buffer is added for the Buffer global, that is done here:

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

Line 82 in 47379bf

"Buffer" => {

LekoArts · 2022-02-23T14:10:03Z

Ok, so I pushed some more changes to the Rust code and I have two problems now:

Actually the generated relative path is incorrect
I'm not seeing my changes in the tests

Let me explain.

When we have a source file at <root>/index.ts and we compile to <root>/dist/index.js we would expect that the relative path to the projectRoot is "..".
But it isn't.

Here is the relevant line: https://github.com/LekoArts/parcel/blob/eca555e83e8ba3ac7ad0eded8dd9c0723a21713e/packages/transformers/js/core/src/global_replacer.rs#L144

dirname here is the path of the <root>/index.ts file, which is identical with projectRoot. So the relative path created will be "" and not "..".

How can I get the distDir here to construct a correct relative path?

I'm not seeing my Rust code changes in the tests. I think I've added the test and the new option correctly, but when I print out the js (currently commented out) I'm not seeing the changes I did when I still had the example project in packages/examples.

LekoArts · 2022-02-28T13:16:13Z

Let me know @mischnic if my above questions/comments make sense and you can help me? Appreciate it a lot! Thanks

mischnic · 2022-02-28T14:34:43Z

How can I get the distDir here to construct a correct relative path?

The transformer doesn't know the final path of the bundle that the asset will be put into. This is only known after bundling/packaging (https://parceljs.org/plugin-system/overview/), so you have to do the path.relative(root, bundle.filePath) computation and insertion in a packager (so @parcel/packager-js).

I don't know why the changes wouldn't show up in the tests (apart from forgetting to recompile the Rust code), so I guess it has to be that your change doesn't modify the behaviour somehow.

LekoArts · 2022-02-28T15:17:19Z

Ok, thanks for that information. I'll look into the tests, I probably made an error somewhere.

I'm trying to wrap my head around how to go about this then. Do I need to keep the changes I made to the transformer so far and add some "identifier" so that later the packager can modify/replace things, or do I completely remove the Rust logic and make all my changes in the packager? From my understand I'd need both, so that in the transformer I actually replace the call and then the packager can change the import path?

mischnic · 2022-02-28T15:22:51Z

Exactly. The logic to determine whether __dirname wasn't declared as a local variable definitely needs to happen in the transformer.

Then in the packager, one possibility would be replacing that placeholder literally or (though somewhat sketchy) do the same as $parcel$global and just assume that this variable is unused by the user and emit it in the transformer, and then prepend a variable declaration in the packager. Though in this case you probably need one variable per asset

parcel/packages/transformers/js/core/src/hoist.rs

Line 835 in 13649c3

return Ident::new("$parcel$global".into(), node.span);

LekoArts · 2022-03-14T09:45:00Z

Ok, I was sick 🤧 for a bit, I'll start working on this again now

LekoArts · 2022-03-18T12:42:37Z

Ok, so I got it working in my example (currently still committed in this PR) in how I expect it:

But I'll remove the example and add tests.

The API and naming etc. is pretty gross and I'm also still using same placeholder everywhere, not per asset.

So it works, but still lot to do.

@mischnic

LekoArts · 2022-04-07T06:21:37Z

Thanks! Will look into it and add a test

LekoArts · 2022-04-07T08:08:24Z

Good catch @mischnic 👍

The issue was that I've used pathdiff::diff_paths for __filename where I didn't need to use it. It just worked in the past but it seems with /folder-name/index.js it doesn't give back index.js but /folder-name/index.js.

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

packages/packagers/js/src/ScopeHoistingPackager.js

mischnic

Looks good to me!

LekoArts · 2022-04-08T07:06:32Z

Awesome 🎉 I'll let you re-run the CI, otherwise with my commit trigger I'd invalidate your review

devongovett · 2022-04-08T14:58:33Z

packages/packagers/js/src/DevPackager.js

+            ),
+          );
+          wrapped = wrapped.replace('$parcel$dirnameReplace', relPath);
+          wrapped = wrapped.replace('$parcel$filenameReplace', relPath);


Is there a reason we have two different variables that get replaced with the same path? Were these meant to be different, or do we only need one of them?

devongovett · 2022-04-08T15:02:00Z

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

+
+          *self.has_node_replacements = true;
+        }
+        "__dirname" => {


Some of this could probably be combined with the above but ok for now

devongovett

Looks good overall! Thanks again!

LekoArts · 2022-04-08T17:29:02Z

I will optimize this (i.e. address the two new review comments) in a follow-up PR -- I'd like to get this into a nightly release so that we can try it out at Gatsby :)

LekoArts · 2022-04-11T11:58:35Z

I think you'll need to re-run a bunch of times for the flaky tests 😅

LekoArts · 2022-04-11T13:53:26Z

With #7931 merged I guess I need to change some ast::Lit::Str occurrences. I'll do this in a bit

mischnic · 2022-04-11T13:57:14Z

Sorry, I forgot that merging it would break this PR 😬

mischnic · 2022-04-11T14:27:26Z

Looks like it compiles, the only breaking change from the swc update was related to :ast:Number. And these changes from ast::Str{} to .into() are just using a shorter syntax and are equivalent. So we can clean that up later.

LekoArts · 2022-04-11T15:22:58Z

Awesome. I'll do that in the follow-up PR. Thanks for merging and help on the PR <3

LekoArts · 2022-04-12T06:50:52Z

The nightly release that would include this failed here: https://github.com/parcel-bundler/parcel/actions/runs/2151865409

initial

2c3b500

LekoArts commented Feb 17, 2022

View reviewed changes

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

LekoArts commented Feb 17, 2022

View reviewed changes

packages/transformers/js/src/JSTransformer.js Outdated Show resolved Hide resolved

LekoArts commented Feb 17, 2022

View reviewed changes

packages/transformers/js/src/JSTransformer.js Outdated Show resolved Hide resolved

LekoArts changed the title ~~feat(@parcel/transformer-js): Rewrite __dirname to be relative to projectRoot~~ feat(@parcel/transformer-js): Rewrite __dirname/__filename to be relative to projectRoot Feb 17, 2022

mischnic reviewed Feb 21, 2022

View reviewed changes

LekoArts added 3 commits February 22, 2022 15:59

revert some changes

c24fccb

add path import

a7e7b3a

test wip

eca555e

Merge branch 'v2' into dirname-relative-path

f3a29cb

LekoArts added 6 commits March 14, 2022 11:50

re-add example

b7b49e3

update swc

8d73fe0

update example

2a45008

move logic to new replacer

08bd388

edit packager

a2d6f12

Merge branch 'v2' into dirname-relative-path

fd1a569

LekoArts added 5 commits March 22, 2022 15:20

delete example once more

a231c46

revert a change

1646589

add js transformer test

87df079

fix filename

e8e9a4d

revert typo

a634a00

LekoArts marked this pull request as ready for review March 22, 2022 15:50

use filename OsStr instead of pathdiff in __filename

997e407

LekoArts and others added 2 commits April 7, 2022 10:27

trigger ci

151a54e

Merge branch 'v2' into dirname-relative-path

0b45828

mischnic reviewed Apr 7, 2022

View reviewed changes

packages/transformers/js/core/src/lib.rs Outdated Show resolved Hide resolved

use simpler fold_with

7d313ca

mischnic reviewed Apr 7, 2022

View reviewed changes

packages/packagers/js/src/ScopeHoistingPackager.js Outdated Show resolved Hide resolved

LekoArts added 2 commits April 7, 2022 12:18

move replace call up in scopeHoistingPackager

b415560

trigger ci

0d96930

mischnic approved these changes Apr 7, 2022

View reviewed changes

Merge branch 'v2' into dirname-relative-path

c7bc555

devongovett reviewed Apr 8, 2022

View reviewed changes

devongovett approved these changes Apr 8, 2022

View reviewed changes

Merge branch 'v2' into dirname-relative-path

ae7ce09

Merge branch 'v2' into dirname-relative-path

43e16a9

mischnic merged commit be0929a into parcel-bundler:v2 Apr 11, 2022

LekoArts deleted the dirname-relative-path branch April 11, 2022 15:23

mischnic mentioned this pull request Apr 13, 2022

Workaround to undo __filename rewriting in parcel/package-manager #7948

Merged

LekoArts mentioned this pull request Apr 26, 2022

__dirname inaccurate in TS gatby-configs gatsbyjs/gatsby#35488

Closed

2 tasks

101arrowz mentioned this pull request Oct 2, 2023

Expose real import.meta properties, plus __dirname and __filename for Node.js #9284

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(@parcel/transformer-js): Rewrite `dirname`/`filename` to be relative to `asset.filePath` #7727

feat(@parcel/transformer-js): Rewrite `dirname`/`filename` to be relative to `asset.filePath` #7727

LekoArts commented Feb 17, 2022 •

edited

Loading

height bot commented Feb 17, 2022

mischnic left a comment •

edited

Loading

LekoArts commented Feb 23, 2022

LekoArts commented Feb 28, 2022

mischnic commented Feb 28, 2022

LekoArts commented Feb 28, 2022

mischnic commented Feb 28, 2022

LekoArts commented Mar 14, 2022

LekoArts commented Mar 18, 2022

LekoArts commented Apr 7, 2022

LekoArts commented Apr 7, 2022

mischnic left a comment

LekoArts commented Apr 8, 2022

devongovett Apr 8, 2022

devongovett Apr 8, 2022

devongovett left a comment

LekoArts commented Apr 8, 2022

LekoArts commented Apr 11, 2022

LekoArts commented Apr 11, 2022 •

edited

Loading

mischnic commented Apr 11, 2022

mischnic commented Apr 11, 2022 •

edited

Loading

LekoArts commented Apr 11, 2022

LekoArts commented Apr 12, 2022

feat(@parcel/transformer-js): Rewrite __dirname/__filename to be relative to asset.filePath #7727

feat(@parcel/transformer-js): Rewrite __dirname/__filename to be relative to asset.filePath #7727

Conversation

LekoArts commented Feb 17, 2022 • edited Loading

↪️ Pull Request

💻 Examples

🚨 Test instructions

✔️ PR Todo

height bot commented Feb 17, 2022

mischnic left a comment • edited Loading

Choose a reason for hiding this comment

LekoArts commented Feb 23, 2022

LekoArts commented Feb 28, 2022

mischnic commented Feb 28, 2022

LekoArts commented Feb 28, 2022

mischnic commented Feb 28, 2022

LekoArts commented Mar 14, 2022

LekoArts commented Mar 18, 2022

LekoArts commented Apr 7, 2022

LekoArts commented Apr 7, 2022

mischnic left a comment

Choose a reason for hiding this comment

LekoArts commented Apr 8, 2022

devongovett Apr 8, 2022

Choose a reason for hiding this comment

devongovett Apr 8, 2022

Choose a reason for hiding this comment

devongovett left a comment

Choose a reason for hiding this comment

LekoArts commented Apr 8, 2022

LekoArts commented Apr 11, 2022

LekoArts commented Apr 11, 2022 • edited Loading

mischnic commented Apr 11, 2022

mischnic commented Apr 11, 2022 • edited Loading

LekoArts commented Apr 11, 2022

LekoArts commented Apr 12, 2022

feat(@parcel/transformer-js): Rewrite `dirname`/`filename` to be relative to `asset.filePath` #7727

feat(@parcel/transformer-js): Rewrite `dirname`/`filename` to be relative to `asset.filePath` #7727

LekoArts commented Feb 17, 2022 •

edited

Loading

mischnic left a comment •

edited

Loading

LekoArts commented Apr 11, 2022 •

edited

Loading

mischnic commented Apr 11, 2022 •

edited

Loading