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

expose transform and resolveapi from parcel #9193

Merged
merged 7 commits into from
Sep 8, 2023

Conversation

kiddkai
Copy link
Contributor

@kiddkai kiddkai commented Aug 15, 2023

↪️ Pull Request

Exposes two new API to public use:

  • Parcel.transform
  • Parcel.resolve

So API consumers can use these two features to reuse the same resolve & transform pipelines
during development.

The implementation is mostly a copy of: v2...parcel-node

But it excluded the features specifically for jest and NodeResolve.
And it does not transform import directly to require that only specific for node register.
As an alternative, we may be able to add another asset query to change it to something else.

eg:

query: `standalone=true&renameImportTo=__custom__import__`

Which provides more customisability, but I am not adding it for now since we still able to post-process the import() for now.

💻 Examples

The new exposed API allows us to do:

let parcel = createParcel({workerFarm});
    let res = await parcel.transform({
      filePath: path.join(__dirname, 'fixtures/parcel/other.js'),
      query: 'standalone=true',
    });
    let code = await res[0].getCode();

    assert(code.includes('require("./index.js")'));
    assert(code.includes('new URL("index.js", "file:" + __filename);'));
    assert(code.includes('import("index.js")'));

And

let res = await parcel.resolve({
        specifier: './other',
        specifierType: 'esm',
        resolveFrom: path.join(__dirname, 'fixtures/parcel/index.js'),
      });
      assert.deepEqual(res, {
        filePath: path.join(__dirname, 'fixtures/parcel/other.js'),
        code: undefined,
        query: undefined,
        sideEffects: undefined,
      });

which allows us to develop custom runtime based on v8 or similar js runtime to share the
transform and resolve pipelines during development without bundling the whole project
which benefits huge projects that takes long time to bundle.

🚨 Test instructions

  • Unit tested.

✔️ 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

Copy link
Contributor

@mattcompiles mattcompiles left a comment

Choose a reason for hiding this comment

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

Couple of questions but this is looking pretty good and love how simple it is to add so far.

@devongovett Would love your thoughts on the comments as well.

}),
});

let res = await this.#requestTracker.runRequest(request, {force: true});
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use force here? My understanding is that force means the cache is ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can update that, but it was originally copied from the intiial branch, I will leave this Q to @devongovett on if this is required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just changed it to a configurable value. Follows the config of shouldCacheDisabled from the main config.
I guess this could be helpful to debug transform problems.

name: 'test',
});

let res = await this.#requestTracker.runRequest(req, {force: true});
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question here about force.


let req = createPathRequest({
dependency,
name: 'test',
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what the purpose of this name is. Looks like it's normally sourced from the AssetGraphBuilder and it could be Main or Runtimes.

@devongovett Any input on what this should be?

Copy link
Contributor Author

@kiddkai kiddkai Aug 16, 2023

Choose a reason for hiding this comment

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

I changed it to .specifeir

The name seem to be used to construct the req.id only. The first part of id is already hashed, so whatever value here doesn't seem to be matter too much.

Tho, change to specifier should be a lil bit more meaningful.

filePath: FilePath,
code?: string,
env?: EnvironmentOptions,
pipeline?: ?string,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if pipeline makes sense as a public option? Shouldn't this be inferred from the .parcelrc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am happy to remove this pipline property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed.

@@ -442,6 +442,7 @@ export default (new Transformer({
is_esm_output: asset.env.outputFormat === 'esmodule',
trace_bailouts: options.logLevel === 'verbose',
is_swc_helpers: /@swc[/\\]helpers/.test(asset.filePath),
standalone: asset.query.has('standalone'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about using a query param to drive this behaviour. Maybe this should be part of the environment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did think about it, for now using query provides some flexibility & less moving part to change the environment. Kinda like a feature flag for jsTransform for now.

});

let res = await this.#requestTracker.runRequest(req, {
force: shouldDisableCache,
Copy link
Contributor

@mattcompiles mattcompiles Sep 6, 2023

Choose a reason for hiding this comment

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

As above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

});

let res = await this.#requestTracker.runRequest(request, {
force: shouldDisableCache,
Copy link
Contributor

Choose a reason for hiding this comment

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

Turns out I was wrong about this and there's a separate cache layer for transforms so this is safe to hardcode as true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

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