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

Rewritten core, bugfixes and ES2015 #19

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Rewritten core, bugfixes and ES2015 #19

wants to merge 10 commits into from

Conversation

izolate
Copy link
Contributor

@izolate izolate commented Feb 7, 2016

Started as basic upgrade to ES2015, but turned into a full re-write as @tellnes suggested in #17. Builds on my previous PR in order to completely replace Jade with Pug. Due to breaking changes, recommend a 2.0.0 major release.

Changes

  • Re-wrote source to remove all Jade dependencies and references in favour of Pug
  • Written source in ES2015, transpiled to ES5 with Babel. Only using ES2015 features that are natively supported in the latest Node.js version (5.x)
  • Dropped support for Jade < 1.0 (added 3 years ago). Reasoning being, Pugify 2.x depends on Pug, which is equivalent to the latest Jade version. Backwards compatibility can be achieved by using Pugify 1.x.
  • Fixed source maps #16 - Seems that jade-code-gen changed syntax, breaking the regex. Unsure if this is the real cause, but my implementation passes tests.
  • Dropped peerDependencies
  • Updated README.md replacing all Jade references with Pug, and examples of updated usage.
  • Added better error handling.

Sorry for the hefty PR. I appreciate your thoughts on this. I'll be sticking around to pick up any issues, especially as the Jade -> Pug transition completes.

As this is now the main pugify on npm, I think we have a moral imperative to keep the codebase clean, up-to-date and bug-free.

}
function generateSourceMaps (src, compiled, file) {
const lines = compiled.split('\n')
const generator = new SourceMapGenerator({ file: `${file}.js` })
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know it was in the old version also, but why do we need to add .js to the filename?

Copy link
Owner

Choose a reason for hiding this comment

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

can't remember why, need to test if browser is happy without .js ( and how file parameter is used )

@tellnes
Copy link
Collaborator

tellnes commented Feb 9, 2016

Added a few comments. Nothing major.

The transformTools.makeStringTransform suggestion can maybe be done in another pr after this one is merged.

This pr also remove the reqguire.extensions registration feature. I'm +1 on doing that, but it should be mentioned.

@herzinger
Copy link

herzinger commented Sep 2, 2017

Sorry to butt in, but is this project active at all? I was meaning to use it, but such a major PR stopped since February 2016 kinda dissuaded me. I'm making a major rewrite of a big project, and was thinking of changing from jade to pug in it, and so going from jadeify to pugify, but this situation smells like trouble like this.

@sidorares
Copy link
Owner

@herzinger it's not very active but I'm keen to maintain it

Do you want to hellp with this pr? To start we need to pull https://github.com/izolate/pugify.git#esnext and rebase it

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.

4 participants