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

Begin process to replace Loom with Turbo #707

Merged
merged 1 commit into from
Mar 27, 2024

Conversation

paulomarg
Copy link
Contributor

WHY are these changes introduced?

We want to revamp our build process for the packages in this repo for a few reasons:

  • Building both ESM and CJS outputs to make it easier for more apps to import the package
  • Building with a target fitting for Node 16 (which we still technically support) to make stack traces clearer
  • Building with turbo + rollup instead of loom (which uses rollup internally) to make it easier for folks to contribute

These changes should have no effect on the final packages, other than the ones stated above.

@paulomarg paulomarg requested a review from a team as a code owner March 20, 2024 20:23

const packageName = pkg.name.substring(1);
export const bannerConfig = {
banner: `/*! ${packageName}@${pkg.version} -- Copyright (c) 2023-present, Shopify Inc. -- license (MIT): https://github.com/Shopify/shopify-app-js/blob/main/LICENSE.md */`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
banner: `/*! ${packageName}@${pkg.version} -- Copyright (c) 2023-present, Shopify Inc. -- license (MIT): https://github.com/Shopify/shopify-app-js/blob/main/LICENSE.md */`,
banner: `/*! ${packageName}@${pkg.version} -- Copyright (c) 2024-present, Shopify Inc. -- license (MIT): https://github.com/Shopify/shopify-app-js/blob/main/LICENSE.md */`,

?

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 think we might want to keep it as it was - technically this is still the same package as before, so it still applies from 2023 onward.

@lizkenyon
Copy link
Contributor

Will any of these changes resolve the down compilation of the async await, like mentioned here?

@paulomarg
Copy link
Contributor Author

paulomarg commented Mar 25, 2024

Will any of these changes resolve the down compilation of the async await, like mentioned here?

Yes, we'll start building with newer targets and move away from es5, which should hopefully create much more readable transpiled code to use more modern features instead (such as async / await as mentioned in the issue).

Copy link

@thomfoolery thomfoolery left a comment

Choose a reason for hiding this comment

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

I haven't top hatted (are we supposed to?) but it looks good.

Just some question inline below,
and why turbo? I've heard mixed things about turbo.
(I'm assuming we mean this turbo)

"es2022",
"esnext.asynciterable"
"esnext.asynciterable",
"ES2021",

Choose a reason for hiding this comment

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

Just curious how you landed on ES2021?

Also curious why its the only one missing from the previous list?

Copy link
Contributor Author

@paulomarg paulomarg Mar 26, 2024

Choose a reason for hiding this comment

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

I was following the TS guide for which target to use for Node 16, which we still technically support. We might want to move that to 2022 when we move on to 18.

Also curious why its the only one missing from the previous list?

Most likely just a mistake on our part, especially since we don't really need to have all of those different lib versions - it might actually be more of a risk if we rely on some invalid types.

@paulomarg
Copy link
Contributor Author

and why turbo?

We've been using turbo successfully on shopify-api-js for a while now, it gives us a good mix of flexibility and ease of use for now. Since this is already going to be a massive change where we're touching every package, I wanted to err on the safe side and not introduce a whole new tool.

We can always migrate off of turbo later once everything is in place, but for now I'd rather focus on getting better output transpiled code without messing with the toolset.

@thomfoolery
Copy link

thomfoolery commented Mar 26, 2024

We've been using turbo successfully on shopify-api-js for a while now

@paulomarg 👍
Sorry, I thought I saw turbo was added as a dependency, didn't realize it was already being used.
Makes sense to stick with what works!

@paulomarg
Copy link
Contributor Author

paulomarg commented Mar 26, 2024

We've been using turbo successfully on shopify-api-js for a while now

@paulomarg 👍 Sorry, I thought I saw turbo was added as a dependency, didn't realize it was already being used. Makes sense to stick with what works!

It was only being used on the other repo so far, but the plan is to unify the build / run tools under turbo / rollup and merge the repos so we only have one repo to maintain.

My plan was to:

  • Change this repo to build with turbo to ensure our settings for the other repo still made sense
  • Get all of the packages building exactly as they were
  • Merge the packages folders from both repos
  • Ensure everything is building
  • Move @shopify/shopify-api away from just tsc and use rollup to have a single process for every package

@paulomarg
Copy link
Contributor Author

I didn't mention that at first, but this PR is the first step in my list above - I have a couple more changes coming that will get every package in this repo building on turbo + other improvements.

Copy link

@thomfoolery thomfoolery left a comment

Choose a reason for hiding this comment

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

LGTM

@paulomarg paulomarg merged commit 926ff7b into build_process_revamp Mar 27, 2024
10 checks passed
@paulomarg paulomarg deleted the replace_loom_with_turbo branch March 27, 2024 14:33
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