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

Transpile all frontity.settings/config imports #837

Merged
merged 4 commits into from Jun 2, 2021

Conversation

luisherranz
Copy link
Member

@luisherranz luisherranz commented May 31, 2021

What:

Change the files that the ts-node that we run in the dev and build commands transpile.

Why:

Because it was not transpiling files that were inside the node_modules folder, like frontity.config.js files in external packages.

How:

I created a regular expression that is passed to the ignores setting of ts-node and makes it transpile only:

  • Packages from the @frontity org
  • Packages with frontity in the name (for example some-frontity-package).
  • frontity.settings files
  • frontity.config files
  • Any TypeScript files that people import.

By default this setting was node_modules.

This ts-node transpilation is only used outside of Webpack, which means mostly things people import in their frontity.settings or frontity.config files. Once Webpack runs, all the files are transpiled.

Tasks:

Unrelated Tasks

  • TSDocs
  • TypeScript
  • End to end tests
  • TypeScript tests
  • Update starter themes
  • Update other packages
  • Update community discussions

@changeset-bot
Copy link

changeset-bot bot commented May 31, 2021

🦋 Changeset detected

Latest commit: 039d6fd

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@luisherranz luisherranz linked an issue May 31, 2021 that may be closed by this pull request
@github-actions
Copy link
Contributor

github-actions bot commented May 31, 2021

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@michalczaplinski
Copy link
Member

I'm not sure why the CLI tests keep failing occasionally and only on MacOS. I thought this should have been fixed in #824.

@luisherranz
Copy link
Member Author

True. It seems like this time was a timeout. We'll keep an eye on them 🙂

I have rerun them and they passed this time.

@luisherranz luisherranz marked this pull request as ready for review June 1, 2021 07:04
@luisherranz luisherranz requested a review from DAreRodz June 1, 2021 07:05
@luisherranz luisherranz self-assigned this Jun 1, 2021
Copy link
Member

@DAreRodz DAreRodz left a comment

Choose a reason for hiding this comment

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

Seems just fine to me. 👍

Although for me it's not clear the part of not ignoring packages with frontity. Would it mean that some frontity packages (those imported by frontity settings/config files) should contain frontity in their name?

@luisherranz
Copy link
Member Author

luisherranz commented Jun 1, 2021

It is a compromise between transpiling everything (which is pretty slow) and not transpiling anything, which could lead to problems like the one this PR is trying to solve. I tried to come up with a set of scenarios where transpiling seems more right than wrong.

EDIT: I think I didn't address your question:

Would it mean that some frontity packages (those imported by frontity settings/config files) should contain frontity in their name?

The answer is no, they don't need to contain frontity in their names for a normal use 🙂

@luisherranz luisherranz merged commit 3b05dac into dev Jun 2, 2021
@luisherranz luisherranz deleted the transpile-external-frontity-configs branch June 2, 2021 07:30
This was referenced Jun 3, 2021
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.

Transpile frontity.config.js files
3 participants